-
-
Notifications
You must be signed in to change notification settings - Fork 32.4k
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
[joy-ui][design] Stray design tweaks to components #38476
Conversation
Netlify deploy previewBundle size reportDetails of bundle changes (Toolpad) |
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.
Great changes all-around ⎯ I love the subtle box shadow on the inputs, it really elevates the design a lot! 🤙 Make sure to check the failing PR tests there, though.
Co-authored-by: Siriwat K <[email protected]> Signed-off-by: Victor Zanivan Monteiro <[email protected]>
Co-authored-by: Siriwat K <[email protected]> Signed-off-by: Victor Zanivan Monteiro <[email protected]>
Signed-off-by: Victor Zanivan Monteiro <[email protected]>
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.
One last question, since the shadow is added to Input, should it be the same for Textarea, Select, Autocomplete?
@zanivan I found an edge case. There are common cases where <Input variant="plain" sx={{ boxShadow: 'none' }} />
<Select variant="plain" sx={{ boxShadow: 'none' }} /> Should the shadow applied only to the outlined variant? |
I think it makes sense—in the same way, I believe we should remove the shadows from outlined, soft, and solid cards. What do you think? |
I think that you mean |
Actually no. Sorry, I expressed myself poorly, but I thought that for the cards, we should only keep the shadows on the plain variant. However, after making some tests, I think we could remove the shadows from all the cards by default. @danilo-leal I'd love to hear your take on this. I've seen many examples in libraries where, by default, the cards have shadows—but, when it comes to real products, it's commonly the opposite. |
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.
This is looking great, overall! Agree with the reflection above ☝️ and it's been real in my experience as well with real apps. It may be generally safer to go without it by default as that can be too opinionated, devs and designers will customize that quickly if they need it!
@zanivan Oh, by the way, one thing I just realized while browsing the Joy UI docs that we could do here as well: I think the Chip default size could also be changed to |
Totally! And I think even the |
We have discussed this and the fix to this is to change the values of |
Co-authored-by: Siriwat K <[email protected]> Signed-off-by: Victor Zanivan Monteiro <[email protected]>
Signed-off-by: Victor Zanivan Monteiro <[email protected]> Co-authored-by: Siriwat K <[email protected]>
Signed-off-by: Victor Zanivan Monteiro <[email protected]> Co-authored-by: Siriwat K <[email protected]>
Added some tweaks to the components, to make them consistent and work better together out-of-the box.
Changes on default Card
md
fromsm
outlined
Changes on default Input
input
Changes on default Alert
soft