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

🪟 🎉 Column selection UI #20267

Merged
merged 18 commits into from
Dec 14, 2022
Merged

🪟 🎉 Column selection UI #20267

merged 18 commits into from
Dec 14, 2022

Conversation

josephkmh
Copy link
Contributor

@josephkmh josephkmh commented Dec 8, 2022

What

Closes https://github.com/airbytehq/airbyte-cloud/issues/3423

Adds the column selection UI to the stream table:

column.selection.ui.mov

How

To support column selection, we are adding two fields to the AirbyteStreamConfiguration object:

export interface AirbyteStreamConfiguration {
  syncMode: SyncMode;
  cursorField?: string[];
  destinationSyncMode: DestinationSyncMode;
  primaryKey?: string[][];
  aliasName?: string;
  selected?: boolean;
  fieldSelectionEnabled?: boolean;        // <-- new field!
  selectedFields?: SelectedFieldInfo[];   // <-- new field!
}

In the stream table, there is a new column - "Sync" - that displays a <Switch /> for each field in the stream:
Screenshot 2022-12-09 at 2 50 02 PM

Nested fields cannot be individually selected/deselected, so their <Switch /> is synced with their parents' value.

Recommended reading order

  1. StreamFieldTable.tsx, FieldRow.tsx, FieldHeader.tsx - where the new UI is actually rendered
  2. CatalogSection.tsx - where the callbacks manipulating the new config object fields are located
  3. The rest

🚨 User Impact 🚨

These changes are currently behind an experiment flag ("connection.columnSelection") that is off by default (and therefore off in OSS). The web API is already accepting the new field selection configuration (#20259) but not yet filtering the fields. Once we test the end to end column feature selection is working properly, we will roll out the feature out to all users.

@octavia-squidington-iv octavia-squidington-iv added area/platform issues related to the platform area/frontend Related to the Airbyte webapp labels Dec 8, 2022
@josephkmh josephkmh marked this pull request as ready for review December 9, 2022 14:03
@josephkmh josephkmh requested a review from a team as a code owner December 9, 2022 14:03
@josephkmh josephkmh changed the title [DRAFT] 🪟 🎉 Column selection UI 🪟 🎉 Column selection UI Dec 9, 2022
@josephkmh josephkmh requested a review from ambirdsall December 9, 2022 16:19
@ambirdsall
Copy link
Contributor

Visual nit: when the checkbox is not being rendered, a tiny thin <label> is still showing a 1px border where it seems like there should be nothing. Also: if you deselect a table, should all of its columns be deselected in the dropdown, too? I can see both sides, aside from complexity, a user might
column-selection-questions

@josephkmh
Copy link
Contributor Author

Also: if you deselect a table, should all of its columns be deselected in the dropdown, too? I can see both sides, aside from complexity, a user might

Good question - I also thought about this and decided to leave the column selection state alone when enabling/disabling a stream entirely. I can imagine that someone might want to temporarily disable a stream, but keep their column selection configuration in place. Also since you can edit other properties of a disabled stream (e.g. sync mode) it seems to follow that you should also be able to change the column selection independent of enabling the stream.

@josephkmh
Copy link
Contributor Author

Visual nit: when the checkbox is not being rendered, a tiny thin is still showing a 1px border where it seems like there should be nothing.

I think this is unrelated to the column selection changes? The bulk changes checkbox is a separate interaction that already exists. Or am I misunderstanding?

@josephkmh josephkmh requested a review from ambirdsall December 14, 2022 19:28
@@ -96,6 +98,32 @@ const CatalogSectionInner: React.FC<CatalogSectionInnerProps> = ({
[updateStreamWithConfig]
);

const numberOfFieldsInStream = Object.keys(streamNode?.stream?.jsonSchema?.properties).length ?? 0;

const onSelectedFieldsUpdate = (selectedFields: SelectedFieldInfo[]) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

A thought: if onSelectedFieldsUpdate took previouslySelectedFields as a second param, instead of just selectedFields, then the special-case props onFirstFieldDeselected and onAllFieldsSelected could go away and and the fact that those are special cases would be more cleanly scoped to this one file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, I think that would indeed be cleaner! As discussed offline, I'll defer this to a follow-up PR

@ambirdsall
Copy link
Contributor

I think this is unrelated to the column selection changes? The bulk changes checkbox is a separate interaction that already exists.

Double-checked on master and the same visual bug exists, so definitely not part of the required scope for this change. Would you want to bundle that in with your a11y changes to CheckBox, or should I cut a new issue?

@josephkmh
Copy link
Contributor Author

@ambirdsall the bug is within the <Checkbox /> component, right? Could definitely bundle it with the a11y issue then IMO!

Copy link
Contributor

@ambirdsall ambirdsall left a comment

Choose a reason for hiding this comment

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

Per offline discussion, we'll defer the onSelectedFieldsUpdate refactoring to a follow-up PR. Everything else looks good to me, and the UI acted as expected in (admittedly cursory) manual testing. Good work 👍

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants