-
Notifications
You must be signed in to change notification settings - Fork 200
fix: height select with tags on mobile #1985
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 GitHub.
|
13dadc7 to
059230c
Compare
059230c to
a76d5ad
Compare
a76d5ad to
6cb5997
Compare
mathildeleg
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.
Just a few questions but otherwise looks good!
| }>` | ||
| border: 1px solid ${({ theme, $hasError }) => ($hasError ? theme.colors.danger600 : theme.colors.neutral200)}; | ||
| ${(props) => { | ||
| switch (props.$size) { |
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.
There's no need for specific css for the size S anymore?
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.
The size S padding is automatically generated with the clearableFieldPaddingStyles util but there's no support for small size with withTags to true 😅 The problem is that there no small version of the Tag object so it's not possible to set both size to S and withTags to true unfortunately 😢
| padding-block: ${props.$withTags ? '0.3rem' : props.theme.spaces[2]}; | ||
| } | ||
| `; | ||
| padding-inline-start: ${(props) => (props.$withTags ? props.theme.spaces[1] : props.theme.spaces[4])}; |
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.
do you need the check here for props.$withTags if you're overriding it below l120?
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.
Very good point, I removed the condition 😊
|
mathildeleg
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.
Looks good ✅



What does it do?
Fixes wrong height of select multiple when there are some tags.
It should be 48px height and not 40px.
Expected:

Current:

Why is it needed?
To have all fields the same height for consistency on mobile
How to test it?
-> The field area should be 48px (with border)
🚀