-
Notifications
You must be signed in to change notification settings - Fork 13.4k
refactor(searchbar): update according to design #30245
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
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
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.
Also the PR title should use refactor since we are updating the styles, not adding them in.
| --leading-icon-padding: #{calc(globals.$ion-scale-400 + globals.$ion-space-200)}; | ||
| --clear-button-padding: #{calc(globals.$ion-scale-400 + globals.$ion-space-200)}; |
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.
Why were these new variables added? We need to be careful when adding new ones because it introduces them to the public so we try to make sure that they're necessary.
If they're to make it easier to keep track of then it might be best to make Sass variables.
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.
These variables were created with the purpose of making the searchbar's start and end paddings be dynamic according to whether it is rendering leading and/or trailing icons. That being said, I don't see a reason why they have to be CSS variables instead of SCSS variables so I'll make that change 👍🏼
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!
Issue number: internal
What is the current behavior?
The searchbar component does not currently match our UX designs for the ionic theme.
What is the new behavior?
Updated the searchbar so that it does:
Does this introduce a breaking change?
Other information