-
Notifications
You must be signed in to change notification settings - Fork 375
fix(search-input): mirror search icon in rtl, fix double border #9602
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
Conversation
|
Preview: https://patternfly-react-pr-9602.surge.sh A11y report: https://patternfly-react-pr-9602-a11y.surge.sh |
| onClick={onSearchHandler} | ||
| isDisabled={isDisabled} | ||
| > | ||
| <ArrowRightIcon /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be updated no what Eric's PR #9609 went in
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like that, what do you think @thatblindgeye and @wise-king-sullyman? Instead of adding the class directly to an icon, use the icon component. It's not hard-coding a class, and if we change the classname or how an icon mirrors/flips, it's an update to the icon component instead of potentially having to update all components that are referencing this class directly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd be in favor of that. I have a similar situation in my DataList PR. It'd also mean using our Icon component in more places as well so could be win-win.
thatblindgeye
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We just need the snapshots updated but otherwise looks good!
tlabaj
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
|
Your changes have been released in:
Thanks for your contribution! 🎉 |
fixes #9600