Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(Search): new Search component #2064

Merged
merged 31 commits into from
Apr 17, 2024

Conversation

YossiSaadi
Copy link
Contributor

@YossiSaadi YossiSaadi requested a review from a team as a code owner April 14, 2024 15:39
@YossiSaadi YossiSaadi force-pushed the feat/yossi/Search-----New-design-Dev-6249458445 branch from 6998d21 to 0481fcc Compare April 14, 2024 15:40
Copy link
Member

@talkor talkor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks really good! See comments

packages/core/src/components/index.js Show resolved Hide resolved
packages/core/src/components/Search/Search.module.scss Outdated Show resolved Hide resolved
Comment on lines +18 to +20
searchIconName = SearchIcon,
clearIconName = CloseSmallIcon,
clearIconLabel = "Clear",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WDYT about giving a single prop for these? e.g. clearIcon: SubIcon
And I wonder if we should allow changing the searchIcon, it can be done in the TextField, why do we need it in both components?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in TextField the icon is not on the left, here you can have both icons simultaneously.
you have a good point since design also asked if we can avoid it
the thing is that we have cases where people changed the search icon and clear icon

and not sure about the single prop? I need to be able to pass label (as people would probably want to translate the label)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean clearIcon={<Icon icon={Clear} aria-label="clear" />} and not sure if possible but we can use the SubIcon type but making the aria-label required in this case.
Plus if designers don't want it and we don't want to support it we can decide here to avoid it

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be an IconButton, though
and it can add complexity
in my best scenario it is removed, but it might be used by current consumers, making it require codemod and to remove ability

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think codemod should prevent us from doing anything
You can use a render prop (e.g. renderClearIcon and provide props like we did in some other component. Just think about a case where users will want to add a onClearIconClick to trigger analytics for example, you'll have to add a prop for it

packages/core/src/components/Search/__stories__/Search.mdx Outdated Show resolved Hide resolved
@YossiSaadi YossiSaadi force-pushed the feat/yossi/Search-----New-design-Dev-6249458445 branch from 0481fcc to 49a3f53 Compare April 17, 2024 10:39
Base automatically changed from chore/yossi/reusable-base-input to master April 17, 2024 11:14
@YossiSaadi YossiSaadi force-pushed the feat/yossi/Search-----New-design-Dev-6249458445 branch 2 times, most recently from 39bf6ec to 6f0d691 Compare April 17, 2024 12:52
@YossiSaadi YossiSaadi force-pushed the feat/yossi/Search-----New-design-Dev-6249458445 branch from 6f0d691 to 1f7d460 Compare April 17, 2024 12:52
…---New-design-Dev-6249458445

# Conflicts:
#	packages/core/src/components/BaseInput/BaseInput.tsx
#	packages/core/src/components/BaseInput/BaseInput.types.ts
#	packages/core/src/components/BaseInput/__tests__/BaseInput.jest.tsx
@YossiSaadi YossiSaadi merged commit 489a374 into master Apr 17, 2024
10 checks passed
@YossiSaadi YossiSaadi deleted the feat/yossi/Search-----New-design-Dev-6249458445 branch April 17, 2024 14:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants