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

🪟 🎨 Show Source-defined cursor and primary key fields on new Stream Details panel #20366

Merged

Conversation

dizel852
Copy link
Contributor

What

Resolves #20227

Based on #20335 (need to be merged first)

How

Show Source-defined Cursor and PK fields on the new Stream Details panel, since we can't change them - they are with disabled state and tooltip.

Notes: tooltip wasn't in the requirements, I've added it since it can be helpful for a user to explain why selected fields can't be changed by providing a learn more link.

Screenshot at Dec 10 05-22-20

@octavia-squidington-iv octavia-squidington-iv added area/platform issues related to the platform area/frontend Related to the Airbyte webapp labels Dec 12, 2022
@dizel852 dizel852 marked this pull request as ready for review December 13, 2022 15:11
@dizel852 dizel852 requested a review from a team as a code owner December 13, 2022 15:11
@krishnaglick
Copy link
Contributor

Can we change the mouse on disabled cursor fields not-allowed?

@dizel852
Copy link
Contributor Author

Can we change the mouse on disabled cursor fields not-allowed?

Sure, will fix that

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.

LGTM

…t-details-table

# Conflicts:
#	airbyte-webapp/src/components/ui/CheckBox/CheckBox.module.scss
#	airbyte-webapp/src/components/ui/CheckBox/CheckBox.tsx
#	airbyte-webapp/src/components/ui/CheckBox/index.stories.tsx
@teallarson teallarson self-requested a review December 13, 2022 21:26
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.

I like the Tooltip! I wonder if it would be helpful to show for all elements in the column (ie: the disabled and off ones too, so a user knows why they can't interact with them)

I also cannot remember where we ended up on with whether to always show these two columns, or only if the sync mode requires a cursor and/or primary key? I see they are always present.

That said, I don't think either of those pieces are blocking this and code lgtm

@dizel852
Copy link
Contributor Author

@tealjulia Thanks for the review!

I like the Tooltip! I wonder if it would be helpful to show for all elements in the column (ie: the disabled and off ones too, so a user knows why they can't interact with them)

I implemented that at first but wasn't sure if that was right, since we put a lot of tooltips in DOM. And most of them won't be shown. Let's confirm that with @edmundito @Upmitt @andyjih

I also cannot remember where we ended up on with whether to always show these two columns, or only if the sync mode requires a cursor and/or primary key? I see they are always present.

That was discussed on call with @Upmitt and @edmundito. As far as I remember, we agreed to show them always(even if sync mode doesn't support them). But let's confirm that with @edmundito @Upmitt @andyjih

…t-details-table

# Conflicts:
#	airbyte-webapp/src/scss/_z-indices.scss
@dizel852 dizel852 requested review from edmundito and removed request for edmundito December 16, 2022 19:24
@dizel852 dizel852 merged commit 7420cd2 into master Dec 16, 2022
@dizel852 dizel852 deleted the vlad/20227-show-predefined-cursor-and-pk-at-details-table branch December 16, 2022 20:28
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 - update predefined cursor and primary fields behavior
4 participants