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

🪟 🎨 Updating styles for PillButton component according to new Figma design changes #19303

Merged
merged 11 commits into from
Dec 9, 2022

Conversation

YatsukBogdan1
Copy link
Contributor

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 component

Screenshot 2022-11-10 at 22 14 02

With 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

@YatsukBogdan1 YatsukBogdan1 requested a review from a team as a code owner November 10, 2022 20:22
Copy link
Contributor

@edmundito edmundito 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 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?

Copy link
Contributor

@dizel852 dizel852 left a 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:

  1. <SyncModeSelect /> - would be great to have an ability to pass any label(component) to we'd like to see, as it was previously

image

  1. <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>
  );
  1. Fix styles - combine divider style with theme
  2. Fix storybook + add example with divider
  3. Fix react "key" error:

image

@octavia-squidington-iv octavia-squidington-iv added area/platform issues related to the platform area/frontend Related to the Airbyte webapp labels Nov 28, 2022
# Conflicts:
#	airbyte-webapp/src/components/ui/PillSelect/PillButton.module.scss
Copy link
Contributor

@dizel852 dizel852 left a comment

Choose a reason for hiding this comment

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

Did another review, and most of the comments are fixed, but some issues still need to be addressed:

  1. React "key" error

image

  1. Select options design: have not found the new design for the sync mode select option, so can we return the old one?
    In PR:

Screenshot at Dec 09 14-45-01

In master:
Screenshot at Dec 09 14-45-14

Copy link
Contributor

@dizel852 dizel852 left a 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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/frontend Related to the Airbyte webapp area/platform issues related to the platform
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update new connection streams table dropdowns
4 participants