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.

Sign up for GitHub

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

Jump to bottom

🪟 🎨 🧹 Migrate Card component to SCSS and refactor #16407

Merged
merged 23 commits into from
Sep 13, 2022
Merged

🪟 🎨 🧹 Migrate Card component to SCSS and refactor #16407

merged 23 commits into from
Sep 13, 2022

Conversation

natalyjazzviolin
Copy link
Contributor

@natalyjazzviolin natalyjazzviolin commented Sep 7, 2022 •
edited
Loading

What

This closes #15790 and should not alter anything visually in the UI.

  1. This refactors the Card component to:
  • Use SCSS instead of styled components
  • Accept a 'title' prop

  1. Refactors the WorkspaceItem component at /workspaces to use an HTML button for better accessibility.

  2. Removes the ContentCard component and replaces all of its instances with Card

  3. Updates styling to use SCSS in some components that use Card where styled components would have no longer worked.

How

Refactors Card and replaces styled components with SCSS.

Recommended reading order

The Card component and its styling:

  1. airbyte-webapp/src/components/base/Card/Card.tsx
  2. airbyte-webapp/src/components/base/Card/Card.module.scss

Where Card is replaced with a button at /workspaces:

  1. airbyte-webapp/src/packages/cloud/views/workspaces/WorkspacesPage/components/WorkspaceItem.tsx
  2. airbyte-webapp/src/packages/cloud/views/workspaces/WorkspacesPage/components/WorkspaceItem.module.scss

Where Card replaces ContentCard:

  1. airbyte-webapp/src/components/CenteredPageComponents/PaddedCard.tsx
  2. airbyte-webapp/src/components/CreateConnectionContent/CreateConnectionContent.tsx
  3. airbyte-webapp/src/components/DeleteBlock/DeleteBlock.tsx
  4. airbyte-webapp/src/components/Modal/Modal.tsx
  5. airbyte-webapp/src/packages/cloud/views/credits/CreditsPage/components/CreditsUsage.tsx
  6. airbyte-webapp/src/packages/cloud/views/workspaces/WorkspacesPage/components/WorkspacesControl.tsx
  7. airbyte-webapp/src/pages/ConnectionPage/pages/ConnectionItemPage/components/StateBlock.tsx
  8. airbyte-webapp/src/pages/ConnectionPage/pages/ConnectionItemPage/components/StatusView.tsx
  9. airbyte-webapp/src/pages/ConnectionPage/pages/ConnectionItemPage/components/TransformationView.tsx
  10. airbyte-webapp/src/pages/ConnectionPage/pages/CreationFormPage/components/ExistingEntityForm.tsx
  11. airbyte-webapp/src/pages/OnboardingPage/components/UseCaseBlock.tsx
  12. airbyte-webapp/src/pages/SettingsPage/pages/ConfigurationsPage/ConfigurationsPage.tsx
  13. airbyte-webapp/src/pages/SettingsPage/pages/SettingsComponents.tsx
  14. airbyte-webapp/src/views/Connector/ConnectorCard/ConnectorCard.tsx
  15. airbyte-webapp/src/views/Connector/ServiceForm/index.stories.tsx

Where Card replaces ContentCard and adds SCSS:

  1. airbyte-webapp/src/views/Connection/CollapsibleCard.module.scss
  2. airbyte-webapp/src/views/Connection/CollapsibleCard.tsx
  3. airbyte-webapp/src/components/ConnectionBlock/ConnectionBlock.module.scss
  4. airbyte-webapp/src/components/ConnectionBlock/ConnectionBlock.tsx

Deletes ContentCard:

  1. airbyte-webapp/src/components/ContentCard/ContentCard.tsx
  2. airbyte-webapp/src/components/ContentCard/index.stories.tsx
  3. airbyte-webapp/src/components/ContentCard/index.tsx
  4. airbyte-webapp/src/components/index.tsx

🚨 User Impact 🚨

No breaking changes.

Tests

Unit

None needed.

@natalyjazzviolin natalyjazzviolin requested a review from a team as a code owner September 7, 2022 19:35
@github-actions github-actions bot added area/platform issues related to the platform area/frontend Related to the Airbyte webapp labels Sep 7, 2022
@natalyjazzviolin natalyjazzviolin changed the title Cleanup card 🪟 🎨 🧹 Migrate Card component to SCSS and refactor Sep 7, 2022
@dizel852 dizel852 mentioned this pull request Sep 8, 2022
Merged
krishnaglick
krishnaglick reviewed Sep 8, 2022
View reviewed changes
Copy link
Contributor

@krishnaglick krishnaglick left a comment

Choose a reason for hiding this comment

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

I was testing this change on the Connection Status view and it seems to have affected how the Error logs expand. Notably it appears to render the text, then do the element expansion.

airbyte-webapp/src/components/base/Card/Card.tsx Outdated
Comment on lines 27 to 32
className={classNames(
className,
styles.container,
fullWidth ? styles.fullWidth : undefined,
withPadding ? styles.withPadding : undefined
)}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
className={classNames(
className,
styles.container,
fullWidth ? styles.fullWidth : undefined,
withPadding ? styles.withPadding : undefined
)}
className={classNames(className, styles.container, {
[styles.fullWidth]: fullWidth,
[styles.withPadding]: withPadding,
})}

airbyte-webapp/src/components/base/Card/Card.tsx Outdated
className={classNames(
styles.title,
lightPadding || !children ? styles.lightPadding : undefined,
roundedBottom ? styles.roundedBottom : undefined
Copy link
Contributor

Choose a reason for hiding this comment

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

Repeat above object pattern here 👍

...bapp/src/packages/cloud/views/workspaces/WorkspacesPage/components/WorkspaceItem.module.scss

.iconColor {
color: #615eff;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

All these free-standing colors make me a touch nervous. Are we sure these colors can't come from _colors.scss?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh I might have missed this one, sorry! I was having trouble importing _colors.scss in a few places, will go back and check.

Copy link
Contributor

@krishnaglick krishnaglick Sep 9, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

If you continue to have issues with it, feel free to shoot me a ping! @use is a little weird at first :)

Edit: Looks like you got it figured out 👍

@natalyjazzviolin
Copy link
Contributor Author

I was testing this change on the Connection Status view and it seems to have affected how the Error logs expand. Notably it appears to render the text, then do the element expansion.

Could you please point me to where this is? I checked out error logs at /workspaces/<id>/connections/<id>/status and don't see a difference from the master branch
Screen Shot 2022-09-09 at 9 46 37 AM

@krishnaglick
Copy link
Contributor

I was testing this change on the Connection Status view and it seems to have affected how the Error logs expand. Notably it appears to render the text, then do the element expansion.

Could you please point me to where this is? I checked out error logs at /workspaces/<id>/connections/<id>/status and don't see a difference from the master branch Screen Shot 2022-09-09 at 9 46 37 AM

https://www.loom.com/share/c265b9bf1d8d40ceb69dc56eb7f34256
It's rendering differently.

@timroes
Copy link
Collaborator

timroes commented Sep 13, 2022

@krishnaglick I've looked at the Loom with Nataly and also tested master vs this branch. I don't think there is any difference in rendering? The text in both cases will fade to opacity of the content at the same time while expanding it having that short moment where it overlaps the elements below. But I see that as well on master as well as in your Loom recording from master? I think it will just feel slightly different depending on the amount of elements below that it might need to push while animating, why it might feel in your recording from this branch slightly slower, but I don't even feel on the recording that it's slightly slower. If you believe there is an issue on this branch that's not master, could you maybe jump on a call with us, trying to demo it?

@timroes
Copy link
Collaborator

timroes commented Sep 13, 2022

@krishnaglick No longer relevant. We found a nice way to fix this. Nataly will create a separate PR for this, since it's already present on master and unrelated to this change, so we don't clutter this PR with it.

@natalyjazzviolin
Copy link
Contributor Author

@krishnaglick we also looked at the failing E2E tests - looks like there are no frontend errors and the test is flaky!

@natalyjazzviolin natalyjazzviolin mentioned this pull request Sep 13, 2022
Merged
@natalyjazzviolin natalyjazzviolin merged commit 8cb3fa4 into master Sep 13, 2022
@natalyjazzviolin natalyjazzviolin deleted the cleanup-card branch September 13, 2022 19:19
@pmossman
Copy link
Contributor

@natalyjazzviolin @krishnaglick fyi I think the Frontend: Build is broken here, I can't build airbyte locally off of latest master

@teallarson
Copy link
Contributor

teallarson commented Sep 13, 2022 •
edited
Loading

Merged a PR to fix: #16663

This was referenced Sep 14, 2022
Closed
Merged
robbinhan pushed a commit to robbinhan/airbyte that referenced this pull request Sep 29, 2022
@natalyjazzviolin @timroes @robbinhan
🪟 🎨 🧹 Migrate Card component to SCSS and refactor (airbytehq#16407)
87e284f 
* Converts worspace item into a button and adds some styling.

* Adds workspace button width and padding.

* Removes border for correct box shadow styling.

* Converts chevron icon from styled components to scss.

* Brakes card, contentcard, stateblock to rebuild styling and props.

* Uses Card in ConnectorCard instead of ContentCard.

* Adds conditional styling to Card.

* Cleans up unused code.

* Adds className override through props.

* Updates existing destination card component.

* Removes styled components from ConnectionBlock.

* Replaces some ContenCards with Cards.

* Replaces all ContentCard components with Card.

* Adds missing padding in StateBlock.

* Fixes linting errors.

* Corrects stylelint errors.

* Reguested changes.

Co-authored-by: Tim Roes <tim@airbyte.io>
jhammarstedt pushed a commit to jhammarstedt/airbyte that referenced this pull request Oct 31, 2022
@natalyjazzviolin @timroes @jhammarstedt
🪟 🎨 🧹 Migrate Card component to SCSS and refactor (airbytehq#16407)
7da006d 
* Converts worspace item into a button and adds some styling.

* Adds workspace button width and padding.

* Removes border for correct box shadow styling.

* Converts chevron icon from styled components to scss.

* Brakes card, contentcard, stateblock to rebuild styling and props.

* Uses Card in ConnectorCard instead of ContentCard.

* Adds conditional styling to Card.

* Cleans up unused code.

* Adds className override through props.

* Updates existing destination card component.

* Removes styled components from ConnectionBlock.

* Replaces some ContenCards with Cards.

* Replaces all ContentCard components with Card.

* Adds missing padding in StateBlock.

* Fixes linting errors.

* Corrects stylelint errors.

* Reguested changes.

Co-authored-by: Tim Roes <tim@airbyte.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@krishnaglick krishnaglick krishnaglick approved these changes

Assignees
No one assigned
Labels
area/frontend Related to the Airbyte webapp area/platform issues related to the platform
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Cleanup Card components in webapp
5 participants
@natalyjazzviolin @krishnaglick @timroes @pmossman @teallarson