feat(admin-ui): AutoComplete + MultiAutoComplete components #4459
Conversation
# Conflicts: # yarn.lock
|
/storybook |
|
Cypress E2E tests have been initiated (for more information, click here). ✨ |
|
Jest tests have been initiated (for more information, click here). ✨ |
|
Cypress E2E tests have been initiated (for more information, click here). ✨ |
|
Cypress E2E tests have been initiated (for more information, click here). ✨ |
|
Cypress E2E tests have been initiated (for more information, click here). ✨ |
|
Jest tests have been initiated (for more information, click here). ✨ |
|
Cypress E2E tests have been initiated (for more information, click here). ✨ |
|
Jest tests have been initiated (for more information, click here). ✨ |
|
Cypress E2E tests have been initiated (for more information, click here). ✨ |
|
🚀 Storybook Preview |
There was a problem hiding this comment.
Huge PR, so far I've only left a couple of questions re commented out code (basically I left 3 questions but all are related to the same thing).
Rn I'm here 😀
Also, here are a couple of additional questions that came out from a bit of manual testing:
- Should this
xicon be here despite the fact nothing has been selected?
- There are IDs visible for short amount of time here:
visible-ids.mov
| @@ -13,7 +13,7 @@ $webiny-theme-light-text-primary-on-background: rgba(0, 0, 0, 0.87); | |||
| $webiny-theme-light-text-secondary-on-background: rgba(0, 0, 0, 0.54); | |||
| $webiny-theme-light-text-hint-on-dark: rgba(255, 255, 255, 0.5); | |||
| $webiny-theme-light-caret-down: url("data:image/svg+xml,%3Csvg%20width%3D%2210px%22%20height%3D%225px%22%20viewBox%3D%227%2010%2010%205%22%20version%3D%221.1%22%20xmlns%3D%22http%3A%2F%2Fwww.w3.org%2F2000%2Fsvg%22%20xmlns%3Axlink%3D%22http%3A%2F%2Fwww.w3.org%2F1999%2Fxlink%22%3E%0A%20%20%20%20%3Cpolygon%20id%3D%22Shape%22%20stroke%3D%22none%22%20fill%3D%22%23000000%22%20fill-rule%3D%22evenodd%22%20opacity%3D%220.54%22%20points%3D%227%2010%2012%2015%2017%2010%22%3E%3C%2Fpolygon%3E%0A%3C%2Fsvg%3E"); | |||
| $webiny-theme-typography-font-family: "Source Sans Pro"; | |||
| //$webiny-theme-typography-font-family: "Source Sans Pro"; | |||
There was a problem hiding this comment.
Hm... why this change?
I guess we'll get back to this once we start wrapping things up?
There was a problem hiding this comment.
While working on this PR, I've noticed that the new Inter font family is often replaced by Source Sans Pro. At the moment, I have decided to comment it out. As you suggested, I added reminder in GH project
| $webiny-theme-typography-subtitle2-font-weight: 600 !default; | ||
|
|
||
| // import required fonts | ||
| @import url("https://fonts.googleapis.com/css?family=Source+Sans+Pro:400,600,700|Open+Sans:300,400,600,700|Source+Code+Pro:400,700"); | ||
| //@import url("https://fonts.googleapis.com/css?family=Source+Sans+Pro:400,600,700|Open+Sans:300,400,600,700|Source+Code+Pro:400,700"); |
There was a problem hiding this comment.
Do we want to clean these lines fully maybe?
Or leave them commented out? If so, I'd add a reminder in GH project.
There was a problem hiding this comment.
Thank you for the suggestion: I have added a reminder in our GH project.
We need to remove all Material Design tokens from our project files. This can be done while "soft refactoring" the admin app, after we finish developing the components library.
| @@ -38,5 +38,5 @@ body { | |||
| --mdc-typography-font-family: #{$mdc-typography-font-family}; | |||
| --mdc-typography-subtitle2-font-weight: #{$mdc-typography-subtitle2-font-weight}; | |||
|
|
|||
| font-family: "Source Sans Pro"; | |||
| //font-family: "Source Sans Pro"; | |||
There was a problem hiding this comment.
Cleanup or leave uncommented?
If you ask me, we can clean these up, and add reminders in GH project if required.
| value: string; | ||
| disabled?: boolean; | ||
| separator?: boolean; | ||
| item?: any; |
There was a problem hiding this comment.
Not sure if we want to go with any here. Any way to improve this? Like make this interface generic?
But ofc, not P1. If it's not worth it at this point, let's just backlog it.
There was a problem hiding this comment.
It is worth it, and I agree it is not a P1.
I logged it...thank you for pointing this out 👍
|
Not sure if this is present in this branch, but in my branch (where I don't have changes from this PR), I have this:
Again, maybe it's fine on your end, but still, please dbl check when you get a chance. Thanks! |
In the "modal branch", the autocomplete component is a mix of "old vs new": Let's look for this behaviour after we merge this PR, I guess it will be fine:
|
There was a problem hiding this comment.
Was doing a bit of manual testing. Works pretty good I'd say :)
Found these though.
- This is only storybook. But the menu gets cut off when trying to test it out. Should-have priority I'd say, not a must-have.
- Saw an error pop up in the console.
- Disabled states look different. That's on purpose or?
# Conflicts: # packages/admin-ui/src/index.ts
Changes
This PR introduces two new components and improves many others
AutoComplete + MultiAutoComplete
These two new components are based on the
Commandcomponent, a wrapper around cmdk. This component can also serve as a foundation forOmniSearchcomponent. Currently, it is kept private and not exposed outside@webiny/admin-ui.Other improvements
Source Sans Profont from the admin, the rest of RMWC SCSS variables are still in place.reset.scssfile.reffrom the component.inputRefprop, improved how the icons position is handled and removed the possibility of decorating the primitive component.textareaRefprop and removed the possibility of decorating the primitive component.How Has This Been Tested?
Manually + Jest