-
Notifications
You must be signed in to change notification settings - Fork 446
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
Fleet UI: Update Dropdown to use react-select 5.4 and other cleanup #24164
Conversation
4841c7a
to
1f937b5
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #24164 +/- ##
==========================================
+ Coverage 54.10% 54.11% +0.01%
==========================================
Files 1580 1581 +1
Lines 150207 150274 +67
Branches 3818 3800 -18
==========================================
+ Hits 81267 81325 +58
- Misses 62220 62227 +7
- Partials 6720 6722 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
👀 |
frontend/pages/admin/UserManagementPage/components/SelectRoleForm/SelectRoleForm.tsx
Show resolved
Hide resolved
frontend/components/forms/fields/DropdownWrapper/DropdownWrapper.tsx
Outdated
Show resolved
Hide resolved
value={getCurrentValue()} | ||
onChange={handleChange} | ||
isDisabled={isDisabled} | ||
menuPortalTarget={document.body} |
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.
Do we want to let this to be defined by the caller?
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.
Could be an optional prop? menuPortalTarget || document.body
as a default?
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.
💯
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.
Out of curiosity, the type signature in the library is menuPortalTarget?: HTMLElement | null;
ref
And it looks like when you pass null
it just appends it to the menu wrapper https://github.com/JedWatson/react-select/blob/master/packages/react-select/src/components/Menu.tsx#L701-L703
I guess not to break this functionality probably want something to the likes of
menuPortalTarget === undefined ? document.body : menuPortalTarget
This is awesome, thank you @RachelElysia |
Issue
Pre-quel to #22789
Description
DropdownWrapper
which uses react-select 5.4, type script, and functional component (vs.Dropdown
which uses react-select 1.3, javascript, and class component)DropdownWrapper
in storybookDropdownWrapper
used in the UI only in dropdowns in creating/editing user modalScreenshots of dropdown
Displays options that are disabled, option that have help text, option that have tooltip
Displays disabled state, error state, dropdown help text
Updated dropdowns in UI
Checklist for submitter
If some of the following don't apply, delete the relevant line.