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

🪟 🎨 New stream details panel #19219

Merged
merged 67 commits into from
Dec 8, 2022

Conversation

dizel852
Copy link
Contributor

@dizel852 dizel852 commented Nov 9, 2022

What

Resolves #18242

How

Adds new Stream Details Panel

Design:
https://www.figma.com/file/liLQhcbpVHiEDW18kiXQe3/01_03_CONNECTIONS?node-id=2376%3A52243
Proto:
https://www.figma.com/proto/liLQhcbpVHiEDW18kiXQe3/01_03_CONNECTIONS?page-id=972%3A36208&node-id=972%3A36212&viewport=446%2C338%2C0.09&scaling=min-zoom&starting-point-node-id=972%3A36212

Screenshot at Nov 11 03-32-40

Reading order

Since our current implementation of <Table/>(link) is pretty hard to reuse(styled-components) and extend in new components, we've made a decision(on daily sync) to create a new one. This also finally gives us the ability to fix typings and existing errors in the future without breaking existing tables.

  • next/StreamFieldsTable/StreamFieldsTableHeader.tsx - actually, we don't need it anymore, since the "streams flow header" become a part of the actual header(thanks to column grouping in react-table link). I've not removed it since I saw it also uses in other PR .
  • /next/NextTable/NextTable.module.scss - I've tried to extract the "base" styles that we use in the <Table /> component. Props like light, highlighted, customPadding, customWidth were not migrated to the new component, since they mostly were used to style the table, and now we can do it in another way
  • Typings still need to be improved @timroes

create the first version of table powered by current implementation of react-table;
@github-actions github-actions bot added area/platform issues related to the platform area/frontend Related to the Airbyte webapp labels Nov 9, 2022
@dizel852 dizel852 marked this pull request as ready for review November 14, 2022 14:32
@dizel852 dizel852 requested a review from a team as a code owner November 14, 2022 14:32
@dizel852 dizel852 changed the title 🪟 🎨 [WIP] New stream details panel 🪟 🎨 New stream details panel Nov 14, 2022
@edmundito edmundito requested review from josephkmh and a team November 14, 2022 15:34
@teallarson teallarson self-requested a review November 14, 2022 17:28
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 awesome! Left a few comments.

One important piece that isn't supported here is scrolling in a long fields list (try PokeAPI).

# Conflicts:
#	airbyte-webapp/src/components/connection/CatalogTree/next/StreamConnectionHeader.module.scss
@dizel852 dizel852 changed the title 🪟 🎨 New stream details panel 🪟 🎨 [WIP] New stream details panel Nov 21, 2022
@dizel852
Copy link
Contributor Author

dizel852 commented Dec 5, 2022

Fixes and updates:
Had a sync with @edmundito and @Upmitt. Discussed what needs to be addressed:

  • fix arrow icon color: change color to grey-200
  • replace faArrowRight icon with the icon Nico has provided in the design
  • add space around the Panel container
  • make the first column and source icon aligned with the toggle button
  • update folder structure
  • fix panel header issues: no_namespace label is not present when there is no namespace; fix sync mode label

Regarding the empty Cursor and PK columns:

Also, regarding case when we have cursor defined(and can't change it):
Fill in the Cursor column with <RadioButton />s, set the "disable" state to them all, and make the only one selected(the defined cursor name)

Since we do not have a style for <RadioButton /> with a disabled state atm, agreed to make it in another PR.
Created an issue to keep it tracked: #20089

image

@dizel852
Copy link
Contributor Author

dizel852 commented Dec 7, 2022

@edmundito regarding explicitly importing React package:
https://reactjs.org/blog/2020/09/22/introducing-the-new-jsx-transform.html#removing-unused-react-imports

Actually, we don't need to do it anymore from React v17.
IMO I personally prefer to import everything explicitly. 🙂

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.

Did a light pass over the code and tried it out locally, things looking good to me.
I would definitely defer to Edmundo for final approval.

@dizel852 dizel852 dismissed stale reviews from matter-q and teallarson December 8, 2022 13:41

All comments are fixed. Got approvals from other reviewers.

@dizel852 dizel852 merged commit 3877df9 into master Dec 8, 2022
@dizel852 dizel852 deleted the vlad/18242-new-streams-details-panel branch December 8, 2022 13:42
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 team/platform-move
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Stream Details Panel in new streams table
5 participants