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

refactor: [M3-6922] - Add Autocomplete Component #9497

Merged
merged 28 commits into from
Sep 8, 2023

Conversation

jaalah-akamai
Copy link
Contributor

@jaalah-akamai jaalah-akamai commented Aug 5, 2023

Description 📝

We want to enhance accessibility and streamline our use of third-party libraries by eliminating our reliance on react-select. This leverages a customized version of MUI Autocomplete which will enable us to gradually replace occurrences of EnhancedSelect across our entire codebase.

Major Changes 🔄

  • Add generic Autocomplete that should fit our needs. We can iterate on this component as we come across edge cases.
  • Added a complete storybook, which also includes an example of how we can transition our Backups and Clone Linode UI to an Autocomplete.
  • Moved LinodeSelect.styles to Autocomplete.styles since they made more sense there.
  • Added basic unit tests
  • Created a new hook for Select All feature

TODO

  • Extend AutocompleteProps from MUI
  • Allow for flags or icons to be displayed (regions)
  • Clean up renderOptions by maybe being more explicit as to what can be passed in

Preview 📷

Single Select Multi-Select
Screenshot 2023-08-05 at 12 18 07 AM Screenshot 2023-08-08 at 10 45 11 PM
Screenshot 2023-08-08 at 10 45 25 PM
Screenshot 2023-08-08 at 10 45 00 PM
autocomplete-multiple.mov
Custom No Options Message Custom Render Options
Screenshot 2023-08-05 at 12 17 54 AM Screenshot 2023-08-05 at 12 17 45 AM
Regions
Screenshot 2023-08-11 at 10 52 00 AM
Separate Options List
Screenshot 2023-09-06 at 11 17 30 AM Screenshot 2023-09-06 at 11 17 34 AM

How to test 🧪

  1. yarn storybook
  2. http://localhost:6006/?path=/docs/components-autocomplete--documentation
Screenshot 2023-08-05 at 12 44 51 AM

@jaalah-akamai jaalah-akamai self-assigned this Aug 5, 2023
@jaalah-akamai jaalah-akamai marked this pull request as ready for review August 5, 2023 04:45
Copy link
Contributor

@abailly-akamai abailly-akamai left a comment

Choose a reason for hiding this comment

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

This is really nice and clean. Great work!

Added some comments to clean things up.

Also, have we discussed the use of the "Autocomplete" VS "Select" nomenclature. I don't think it is a crucial discussion but I'd be curious to know why gearing towards the MUI naming convention rather than keeping our existing one

value: string;
}

export interface AutocompleteProps {
Copy link
Contributor

Choose a reason for hiding this comment

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

You are overriding a lot of MUI Autocomplete props which I think we should avoid to prevent breaking changes when updating MUI. Rather we should extend the MUI component's props and only declare our custom props. (same with other interfaces below)

They will be automatically picked up by storybook

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Consider this done!

sx,
} = props;

const [inputValue, setInputValue] = React.useState('');
Copy link
Contributor

Choose a reason for hiding this comment

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

Out of curiosity, can the value be an object if (for instance) there's an icon to be displayed with the label and in the selected state? I assume this could be also an iteration

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm assuming you mean like Regions selector? If so, I added an example of how to do that via the optional data property

Copy link
Contributor

Choose a reason for hiding this comment

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

To clarify, this should already be working for the selected option too? I might be missing something but in Storybook it doesn't seem like the flag gets displayed upon selecting an option.

)}
renderOption={(props, option: OptionsType, { selected }) => {
return (
<li {...props}>
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we just pass the props we need here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We're type-checking that only list item properties are applied here, I'd rather keep it for flexibility.

@jaalah-akamai
Copy link
Contributor Author

This is really nice and clean. Great work!

Added some comments to clean things up.

Also, have we discussed the use of the "Autocomplete" VS "Select" nomenclature. I don't think it is a crucial discussion but I'd be curious to know why gearing towards the MUI naming convention rather than keeping our existing one

Yes, I thought about this too! The existing EnhancedSelect allows for typeahead interactions, so that falls within the Autocomplete nomenclature

@tyler-akamai
Copy link
Contributor

Great work!

@jaalah-akamai jaalah-akamai marked this pull request as draft August 8, 2023 21:41
@jaalah-akamai
Copy link
Contributor Author

Sorry @tyler-akamai been making major changes to this PR, I'm going to close out-of-date comments, but looking forward to your review when I put it up again!

@jaalah-akamai jaalah-akamai marked this pull request as ready for review August 11, 2023 03:30
@jaalah-akamai
Copy link
Contributor Author

There's still a lot more unit tests we can write for this. I may tackle writing them in a separate PR due to my time constraints or someone on the team can pick that up. Ready for review again! 🚀

Copy link
Contributor

@dwiley-akamai dwiley-akamai left a comment

Choose a reason for hiding this comment

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

This is looking good based on my testing 🎉

We discussed on Friday during the group review/testing that some improvements can be made for the types (e.g. the one marked TODO in Autocomplete.tsx), but I think that could be done in a follow-up PR or iteratively as we implement this component in the app.

sx,
} = props;

const [inputValue, setInputValue] = React.useState('');
Copy link
Contributor

Choose a reason for hiding this comment

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

To clarify, this should already be working for the selected option too? I might be missing something but in Storybook it doesn't seem like the flag gets displayed upon selecting an option.

@jaalah-akamai
Copy link
Contributor Author

Added a storybook example for having separate options as opposed to using Tags.

@dwiley-akamai Adding flags to the value is not something we can do easily given the types we're restricted to. This may be one tradeoff unless we do a customized input.

@bnussman-akamai
Copy link
Member

Looks great but I have some general concerns about the typesafety of this component.

  • No type safety on the data
  • renderInput is required
  • The onSelectionChange value is an array even though multiple is false

Screenshot 2023-09-06 at 12 50 39 PM

None of this stuff is easy but I propose we try to align our component more with MUI's implementation

  • Allow options to be any type T
  • Use onChange instead of a custom onSelectionChange
  • And so on...

@@ -662,7 +667,6 @@ export const lightTheme: ThemeOptions = {
backgroundColor: 'transparent',
color: primaryColors.main,
},
padding: 12,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was a bug that prevented the IconButton size prop from working properly. There should be no regressions since we default to the large size which has a padding of 12 by default. https://github.com/linode/manager/pull/9497/files#diff-f767369a01cd40921dc0da8dac44cf6777e3c155f3680aa2990be0da93c2dc48R659

@jaalah-akamai
Copy link
Contributor Author

Thanks @bnussman-akamai for the review and providing an alternative solution 🎉

Copy link
Member

@bnussman-akamai bnussman-akamai left a comment

Choose a reason for hiding this comment

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

I think this is a great starting point! 🎉 We can refine this component as we use it and see what changes it may need

@bnussman-akamai bnussman-akamai added Approved Multiple approvals and ready to merge! and removed Ready for Review labels Sep 8, 2023
@jaalah-akamai jaalah-akamai merged commit ac497b7 into linode:develop Sep 8, 2023
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Approved Multiple approvals and ready to merge! Storybook
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants