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

🪟 🐛 Long source and destination names make the connection list table scroll horizontally #21759

Conversation

matter-q
Copy link
Contributor

What

Closes #21274

How

  • Added responsive columns
  • Created new cell components
  • Removed styled-components
  • Refactored Table component

Note: This PR is a temporary solution that fixes some bugs for this moment before we can start using the new Table component

Loom

https://www.loom.com/share/a442f62c9dd1406691b7e4af19d89c40

@octavia-squidington-iv octavia-squidington-iv added the area/frontend Related to the Airbyte webapp label Jan 23, 2023
@matter-q matter-q force-pushed the mark/long-source-and-destination-names-make-the-connection-list-table-scroll-horizontally branch 2 times, most recently from 5a441d5 to a6cccc1 Compare January 24, 2023 03:40
@dizel852 dizel852 changed the title Mark/long source and destination names make the connection list table scroll horizontally 🪟 🐛 Long source and destination names make the connection list table scroll horizontally Jan 24, 2023
Comment on lines 75 to 69
style={{
paddingLeft: column.customPadding?.left ?? SPACING_LG,
paddingRight: column.customPadding?.right ?? SPACING_LG,
width: column.customWidth ? `${column.customWidth}%` : column.collapse ? "0.0000000001%" : "auto",
}}
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm.. just out of curiosity - why do we apply styling in the style prop instead of className?

Copy link
Contributor Author

@matter-q matter-q Jan 25, 2023

Choose a reason for hiding this comment

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

Table is a shared component, just saved original logic (the same as we had with styled-components). The table supports those custom styles...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed padding parameters since we do not use them at all

Comment on lines 106 to 102
style={{
paddingLeft: cell.column.customPadding?.left ?? SPACING_LG,
paddingRight: cell.column.customPadding?.right ?? SPACING_LG,
width: cell.column.customWidth
? `${cell.column.customWidth}%`
: cell.column.collapse
? "0.0000000001%"
: "auto",
}}
Copy link
Contributor

Choose a reason for hiding this comment

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

The same question here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The same answer from above 🙂

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 a review: changes looks great 🔥
Left some comments.
Also noticed some minor issues, would be great to fix them in scope o this PR:

  1. font-weight is changed from 500 to 400. By design - we need 500.

Screenshot at Jan 25 14-37-31

Screenshot at Jan 25 14-38-14

  1. Status icon stretches:

Screenshot at Jan 25 14-26-09

Screenshot at Jan 25 14-25-52

@matter-q matter-q force-pushed the mark/long-source-and-destination-names-make-the-connection-list-table-scroll-horizontally branch from 2677bc6 to f0c98d7 Compare January 25, 2023 21:17
@matter-q matter-q requested a review from dizel852 January 26, 2023 15:03
@matter-q matter-q force-pushed the mark/long-source-and-destination-names-make-the-connection-list-table-scroll-horizontally branch from f0c98d7 to 6f0de4d Compare January 26, 2023 15:10
@matter-q
Copy link
Contributor Author

matter-q commented Jan 26, 2023

Did a review: changes looks great 🔥 Left some comments. Also noticed some minor issues, would be great to fix them in scope o this PR:

  1. font-weight is changed from 500 to 400. By design - we need 500.
Screenshot at Jan 25 14-37-31 Screenshot at Jan 25 14-38-14
  1. Status icon stretches:
Screenshot at Jan 25 14-26-09 Screenshot at Jan 25 14-25-52

Thanks for the detailed review. Fixed with font-weight 500 and width of icon 😉

@dizel852 dizel852 removed the request for review from YatsukBogdan1 January 27, 2023 10:26
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.

LGTM 👍

Passing to @edmundito and @tealjulia for another review

@tealjulia I think you will be interested to look, since pretty close issue was created recently - https://github.com/airbytehq/airbyte/issues/21941

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.

Changes look good. Tested connections, sources, and destinations lists, and resized windows. Noted a couple of nits that may be nice to fix but no need for re-approval.

Copy link
Contributor

@teallarson teallarson left a comment

Choose a reason for hiding this comment

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

Looks like there is a small visual regression on the Credits Page in the CreditConsumptionPerConnectionTable:

Screenshot 2023-01-27 at 10 44 15 AM

(there is now a double card)

@teallarson
Copy link
Contributor

teallarson commented Jan 27, 2023

@tealjulia I think you will be interested to look, since pretty close issue was created recently - https://github.com/airbytehq/airbyte-internal-issues/issues/1316

This mismatch in padding/alignment of the headers and the inconsistent header highlighting were the main two things I was getting at in that issue. I don't think that should be part of this PR.
Screenshot 2023-01-27 at 10 42 52 AM

Mark Berger added 4 commits January 29, 2023 22:33
…ll horizontally

- Removed styled-components
- Refactored Table component
- Added responsive columns
- Created new cell components
…ll horizontally

- Removed styled-components
- Refactored Table component
- Added responsive columns
- Created new cell components
…ll horizontally

- Removed styled-components
- Refactored Table component
- Added responsive columns
- Created new cell components
…ll horizontally

- Removed styled-components
- Refactored Table component
- Added responsive columns
- Created new cell components
@matter-q matter-q force-pushed the mark/long-source-and-destination-names-make-the-connection-list-table-scroll-horizontally branch from 8691621 to 7376885 Compare January 29, 2023 20:33
@matter-q matter-q requested a review from teallarson January 30, 2023 14:32
Copy link
Contributor

@teallarson teallarson left a comment

Choose a reason for hiding this comment

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

Retested the credits and connections pages locally. LGTM.

@matter-q matter-q merged commit 98054d6 into master Jan 30, 2023
@matter-q matter-q deleted the mark/long-source-and-destination-names-make-the-connection-list-table-scroll-horizontally branch January 30, 2023 17:27
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Long source and destination names make the connection list table scroll horizontally
5 participants