Skip to content
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

feat(admin-ui): AutoComplete + MultiAutoComplete components #4459

Open
wants to merge 42 commits into
base: feat/new-admin-ui
Choose a base branch
from

Conversation

leopuleo
Copy link
Contributor

@leopuleo leopuleo commented Dec 20, 2024

Changes

This PR introduces two new components and improves many others

AutoComplete + MultiAutoComplete

These two new components are based on the Command component, a wrapper around cmdk. This component can also serve as a foundation for OmniSearch component. Currently, it is kept private and not exposed outside @webiny/admin-ui.

Other improvements

  1. Removed Source Sans Pro font from the admin, the rest of RMWC SCSS variables are still in place.
  2. Removed reset.scss file.
  3. Separator: removed ref from the component.
  4. Input: added inputRef prop, improved how the icons position is handled and removed the possibility of decorating the primitive component.
  5. Select: chores
  6. Textarea: added added textareaRef prop and removed the possibility of decorating the primitive component.

How Has This Been Tested?

Manually + Jest

@leopuleo leopuleo self-assigned this Dec 20, 2024
@leopuleo leopuleo requested a review from adrians5j December 20, 2024 16:26
@leopuleo leopuleo marked this pull request as ready for review December 20, 2024 16:26
@leopuleo
Copy link
Contributor Author

/storybook

@webiny webiny deleted a comment from github-actions bot Dec 20, 2024
@webiny webiny deleted a comment from github-actions bot Dec 20, 2024
Copy link

Cypress E2E tests have been initiated (for more information, click here). ✨

@webiny webiny deleted a comment from github-actions bot Dec 20, 2024
Copy link

Jest tests have been initiated (for more information, click here). ✨

Copy link

Cypress E2E tests have been initiated (for more information, click here). ✨

Copy link

Cypress E2E tests have been initiated (for more information, click here). ✨

Copy link

Cypress E2E tests have been initiated (for more information, click here). ✨

Copy link

Jest tests have been initiated (for more information, click here). ✨

Copy link

Cypress E2E tests have been initiated (for more information, click here). ✨

Copy link

Jest tests have been initiated (for more information, click here). ✨

Copy link

Cypress E2E tests have been initiated (for more information, click here). ✨

Copy link

🚀 Storybook Preview
You can preview the Storybook by visiting the link below:

Storybook Preview URL

Copy link
Member

@adrians5j adrians5j left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😀
image

Also, here are a couple of additional questions that came out from a bit of manual testing:

  1. Should this x icon be here despite the fact nothing has been selected?

image

  1. 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";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm... why this change?

I guess we'll get back to this once we start wrapping things up?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to clean these lines fully maybe?

Or leave them commented out? If so, I'd add a reminder in GH project.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cleanup or leave uncommented?

If you ask me, we can clean these up, and add reminders in GH project if required.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

value: string;
disabled?: boolean;
separator?: boolean;
item?: any;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is worth it, and I agree it is not a P1.

I logged it...thank you for pointing this out 👍

@adrians5j
Copy link
Member

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:

image

Again, maybe it's fine on your end, but still, please dbl check when you get a chance. Thanks!

@leopuleo
Copy link
Contributor Author

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:

image

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":
the Autocomplete component uses the Input (NEW) and the List (OLD) as primitive components.

Let's look for this behaviour after we merge this PR, I guess it will be fine:

CleanShot 2024-12-31 at 10 20 17@2x

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants