-
-
Notifications
You must be signed in to change notification settings - Fork 321
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
[ui] New component catalog #1003
Conversation
Your Render PR Server URL is https://toolpad-pr-1003.onrender.com. Follow its progress at https://dashboard.render.com/web/srv-cch4cuirrk02bi4tqce0. |
packages/toolpad-app/src/toolpad/AppEditor/PageEditor/ComponentCatalog/ComponentIcon.tsx
Outdated
Show resolved
Hide resolved
packages/toolpad-app/src/toolpad/AppEditor/PageEditor/ComponentCatalog/ComponentCatalogItem.tsx
Outdated
Show resolved
Hide resolved
packages/toolpad-app/src/toolpad/AppEditor/PageEditor/ComponentCatalog/ComponentCatalogItem.tsx
Outdated
Show resolved
Hide resolved
packages/toolpad-app/src/toolpad/AppEditor/PageEditor/ComponentCatalog/ComponentCatalogItem.tsx
Outdated
Show resolved
Hide resolved
packages/toolpad-app/src/toolpad/AppEditor/PageEditor/ComponentCatalog/ComponentCatalogItem.tsx
Outdated
Show resolved
Hide resolved
Your Render PR Server URL is https://toolpad-pr-1003.onrender.com. Follow its progress at https://dashboard.render.com/web/srv-ccl9oqta4990ukkqsd40. |
packages/toolpad-app/src/toolpad/AppEditor/PageEditor/ComponentCatalog/ComponentCatalog.tsx
Show resolved
Hide resolved
Few small points left: |
Looking great overall! 👏
|
The issue seems to be that we don't currently have the Table icon in the
I think it's not too much effort to make it collapsible, and in doing so, it feels to me more appropriate to switch the order of the items in the sidebar like so:
I'm not sure if the hover effect in this case should make it lighter: Screen.Recording.2022-10-11.at.3.05.07.PM.movor darker: Screen.Recording.2022-10-11.at.3.07.42.PM.movgiven that while the components are disabled, the button itself does trigger an action (of redirecting to the GitHub issue) Did you mean adding a background across the entire section, like so: I'll go ahead with adding a background to the entire section, combined with a darker background on hover, subject to correction. |
Hmm, why would there be a discrepancy between this and the Material Icons? Is it possible to add it?
Great! Thank you :D
Thank you! For the most part, darker seems to be the usual pattern for hover effects. This screenshot looks like the hover is quite dark—curious what's your system for choosing the gray values? |
borderStyle: kind === 'create' ? 'dashed' : 'solid', | ||
color: 'text.secondary', | ||
'&:hover': { | ||
backgroundColor: 'action.disabled', |
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 looks right to me:
backgroundColor: 'action.disabled', | |
backgroundColor: 'action.hover', |
It adds a black background with opacity, so it should be just additive with whatever is underneath. For future reference, I'd say adding action.disabled
to a hover effect can be categorized as a code smell. And if we have to do it to make things look right, that'd probably be something that needs to fixed in the theme. Imagine a maintainer working on the theme in 6 months from now finding out we've been using action.disabled
for all kinds of things other than disabled states.
I think this is to do with the icon set being supported by
Update this based on #1003 (comment) to have a lighter background on hover and a slightly darker background overall: |
borderWidth: 1, | ||
borderStyle: 'inset', | ||
borderColor: 'divider', | ||
backgroundColor: 'grey.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.
Will this work with a dark theme?
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've made some updates to this PR to handle the new theme on the catalog when it gets merged.
Screen.Recording.2022-10-14.at.4.29.42.PM.mov
The issue is that given that they are independent pull requests, I can't seem to make any change that is dependent on the other to either pull request.
I think the solution is to merge this first, pull these changes into #988 and then handle any other changes there.
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'm mainly wondering whether it's possible to make this more theme-agnostic? Instead of specifying a specific color, rather darken/lighten the background. Or darken/lighten a theme color.
Testing whether a generic dark theme produces acceptable results is just a matter of switching the palette mode.
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.
Agreed, I've made a change to use darken
which is theme agnostic and does the job:
Screen.Recording.2022-10-18.at.2.18.38.PM.mov
Also see mui/material-ui#32846 (cc @oliviertassinari ) |
Your Render PR Server URL is https://toolpad-pr-1003.onrender.com. Follow its progress at https://dashboard.render.com/web/srv-cd75lc9a6gdukrlliimg. |
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.
👍 Alright, let's go
Closes Component discoverability in catalog #598
Visual demonstration
Screen.Recording.2022-09-15.at.2.49.22.AM.mov