-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
🪟 🎨 Show Source-defined cursor
and primary key
fields on new Stream Details panel
#20366
Conversation
add disabled state style
add disabled state style add small size style add storybook
…predefined-cursor-and-pk-at-details-table
… disable them if they are source-defined
Can we change the mouse on disabled cursor fields |
Sure, will fix that |
There was a problem hiding this 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
There was a problem hiding this 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
@tealjulia Thanks for the review!
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
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
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.