-
Notifications
You must be signed in to change notification settings - Fork 2
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
chore: Group URLs in Select component #443
Conversation
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.
Seems to work well 🙌
A created threshold disappears on closing and reopening the popover but maybe it's expected at this stage.
I would also consider calling the last field in the table STOP TEST
like in the cloud or adding a tooltip because on its own it seem confusing being just stop 🤔
acc.push({ | ||
label: host, | ||
value: host, | ||
disabled: true, | ||
}) |
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.
Semantically, those will still be disabled options, rather than groups.
I suggest one of the following options instead:
StyledReactSelect
that @e-fisher added recently? It's a wrapper aroundreact-select
, which supports groups out of the box.- Use
<Select.Group>
for groups
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.
@going-confetti I've had some issues with ControlledReactSelect
(which implements StyledReactSelect
) inside the PopoverDialog
. The positioning doesn't work well and when trying to use a portal to render it somewhere else, most of the styles were lost, so I followed your second option here by adding support to Select.Group
in the existing ControlledSelect
component.
? 'var(--gray-6)' | ||
: 'var(--gray-3)'}; | ||
color: var(--sand-12); | ||
font-size: var(--font-size-1); |
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.
IMO this should have the same font size as the options, otherwise it looks inconsistent. Though this comment will likely be irrelevant once the other one has been resolved
@Llandy3d This is expected at this point, but will be fixed soon ⚔️ 🐛
I'll do a UI polishment before promoting the feature and made notes of this to address later on! 👍 |
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.
LGTM!
Description
This PR maps the list of allowed requests into a dropdown, so the user can select a URL when defining a new threshold.
How to Test
Checklist
npm run lint
) and all checks pass.npm test
) and all tests pass.Screenshots (if appropriate):
Related PR(s)/Issue(s)