-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
🪟 🎨 Updating styles for PillButton component according to new Figma design changes #19303
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.
This is looking good so far. I left a few comments with areas that could be improved.
Could you also add a new storybook state to show multiple components?
airbyte-webapp/src/components/ui/PillSelect/PillButton.module.scss
Outdated
Show resolved
Hide resolved
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.
Tested locally, and here is my thoughts:
<SyncModeSelect />
- would be great to have an ability to pass any label(component) to we'd like to see, as it was previously
<PillButton />
- there is no need to replace "children" prop with custom one. We lose the ability to compose components. We can pass an array of labels as children. It will be look like, (rough example):
// PillSelect.tsx
return (
<Tooltip
control={
<PillButton
variant={variant}
.......
>
{value.label}
</PillButton>
}`
// PillButton.tsx
const arrayChildren = Children.toArray(children);
return (
<button type="button" {...buttonProps} className={buttonClassName}>
{Children.map(arrayChildren, (child, index) => (
<>
<div className={styles.labelContainer}>
<Text as="span" size="xs" className={styles.text}>
{child}
</Text>
</div>
.........
</>
))}
.......
</button>
);
- Fix styles - combine divider style with theme
- Fix storybook + add example with divider
- Fix react "key" error:
airbyte-webapp/src/components/connection/CatalogTree/next/SyncModeSelect.tsx
Show resolved
Hide resolved
# Conflicts: # airbyte-webapp/src/components/ui/PillSelect/PillButton.module.scss
…llSelect isMulti: true and have labels as array; Updates DropDown Option component to properly render label with array value
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.
Tested locally - "key" error is gone.
Also discusses dropdown options style with @YatsukBogdan1 and @edmundito separately:
this will be fixed in a separate PR
Passing to @edmundito for final review
What
Closes #18243
Figma: https://www.figma.com/file/liLQhcbpVHiEDW18kiXQe3/01_03_CONNECTIONS?node-id=972%3A36208
How
This PR adds new styles for
PillButton
componentWith current implementation (not sure we need this or not), we can basically add any number of labels, to render with divider. Currently it looks like this.
https://www.loom.com/share/a444afb568aa4cdebec957ab6ca8fabc