feat: orgs IDP sync - add combobox to select claim field value when sync field is set #16335
Conversation
b1af4b9 to
e61b2dc
Compare
| @@ -65,7 +66,7 @@ export const SelectScrollDownButton = React.forwardRef< | |||
| )} | |||
| {...props} | |||
| > | |||
| <ChevronDown className="size-icon-sm" /> | |||
| <ChevronDown className="size-icon-sm cursor-pointer text-content-secondary hover:text-content-primary" /> | |||
There was a problem hiding this comment.
lot of duplication between this and the ChevronDown 35 lines up
There was a problem hiding this comment.
Thats sort of the way of Tailwind, duplication of styles is ok to a certain extent. It would feel strange to create a separate component just because 2 icons have the same styles.
There was a problem hiding this comment.
doesn't need to be a component, it could be a string constant or something
There was a problem hiding this comment.
Extracting classNames into strings is a pattern that I would argue we don't want to go down. I follow the idealogy that it is sometimes ok to have repeated classNames until it makes sense to use some technique like creating a new component to manage the duplication. https://v3.tailwindcss.com/docs/reusing-styles
site/src/pages/DeploymentSettingsPage/IdpOrgSyncPage/IdpOrgSyncPage.tsx
Outdated
Show resolved
Hide resolved
site/src/pages/DeploymentSettingsPage/IdpOrgSyncPage/IdpOrgSyncPageView.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Nice!
Just two thoughts:
- do the icons have the same size and stroke width? The check seems slightly smaller
- how come that the bg color of the main drop-down is different for these both cases? (referring to "engineering"). Would suggest to remove the bg color of the second screenshot
|
@chrifro The difference is that IdP organization name is a combobox which means that the top part with the chevron down is actually a button and its not possible to type directly there. The button hover color is being applied because my mouse was hovering the button in the screenshot. Should I create a special case for this button so it doesn't get highlighted on hover? The icons are all using the same settings, so far I do not modify stroke width from the default. |
contributes to coder/internal#330
For organizations IdP sync:
Tests will be added in a separate PR
The same functionality for Group and Role sync will be handled in a separate PR.