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 connection page design #9025

Merged
merged 50 commits into from
Feb 21, 2022
Merged

Conversation

jamakase
Copy link
Contributor

@jamakase jamakase commented Dec 22, 2021

What

Includes a bunch of improvements for Connection Form:

Closes #9728
Closes #9729
Closes #4226
Closes #9462

What is left:

  • Search section for primary key
  • Search in PathPopout ( primary key and cursor field )
  • Update onboarding
  • Migrate table to react-table ( for better support of tables and proper column resizes )
  • Minor designs update ( checkbox positioning, paddings in table )

Recommended reading order

  1. ConnectionForm.tsx
  2. CollapsibleCard.tsx
  3. FormCard.tsx
  4. services/BulkEdit/BulkEditService.tsx
  5. BulkEditHeader.tsx
  6. PathPopout.tsx

This change is Reviewable

@github-actions github-actions bot added area/frontend area/platform issues related to the platform labels Dec 22, 2021
@jamakase jamakase temporarily deployed to more-secrets December 23, 2021 21:57 Inactive
@jamakase jamakase temporarily deployed to more-secrets December 25, 2021 21:18 Inactive
@jamakase jamakase temporarily deployed to more-secrets January 6, 2022 12:04 Inactive
@jamakase jamakase temporarily deployed to more-secrets January 12, 2022 22:44 Inactive
@jamakase jamakase temporarily deployed to more-secrets January 13, 2022 09:08 Inactive
@jamakase jamakase temporarily deployed to more-secrets January 13, 2022 19:49 Inactive
@jamakase jamakase temporarily deployed to more-secrets January 13, 2022 21:35 Inactive
@jamakase
Copy link
Contributor Author

  • 🐛 When the "Replication frequency" is set to "Manual" it will now always clear the select to an empty "..." instead of showing manual.
  • Since most of the settings are now moved to the "Replication" and "Transformation" page, let's change the "gear icon" link in the connections overview table to lead to the replication page instead of the settings page, which now only holds the delete button.

Addressed.

  • We should have a message if the Transformation section is empty, stating that this connection doesn't support any transformations.
    image

@jamakase jamakase temporarily deployed to more-secrets February 17, 2022 20:17 Inactive
@jamakase jamakase temporarily deployed to more-secrets February 18, 2022 12:56 Inactive
@jamakase jamakase temporarily deployed to more-secrets February 18, 2022 13:28 Inactive
@jamakase jamakase temporarily deployed to more-secrets February 20, 2022 19:18 Inactive
@jamakase jamakase temporarily deployed to more-secrets February 20, 2022 19:43 Inactive
@jamakase jamakase temporarily deployed to more-secrets February 20, 2022 19:43 Inactive
Comment on lines +50 to +52
: !["object", "array"].includes(typeof props.data.label)
? props.data.label
: `select_value_${props.data.value}`;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd suggest we never try to attach the props.data.label as a testid here. Since this is i18n, and we might want to run tests later against different or psuedo i18n, I think if testId isn't specified we should directly fallback to the value instead, and use something like lodash's camelCase to make sure we're getting rid of spaces in it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's exactly the reason why I added this logic. By default, we were setting data-testid to label, so if we change it right now to value - it will break some e2e tests.

Should we open separate task for it? Or should I fix it as part of this PR?

Copy link
Contributor

@timroes timroes Feb 21, 2022

Choose a reason for hiding this comment

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

Let's push it into a separate PR. We could potentially also just enforce a testid for all input if we anyway do it in a separate PR.

@jamakase jamakase temporarily deployed to more-secrets February 21, 2022 10:36 Inactive
@jamakase jamakase temporarily deployed to more-secrets February 21, 2022 10:36 Inactive
@jamakase jamakase temporarily deployed to more-secrets February 21, 2022 11:02 Inactive
@jamakase jamakase temporarily deployed to more-secrets February 21, 2022 11:02 Inactive
Copy link
Contributor

@timroes timroes left a comment

Choose a reason for hiding this comment

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

Tested this on Chrome Linux. Functionality wise seems to work. I've mostly have minor concerns around design and code left, but we should not block this PR now on this, and fix them afterwards. So LGTM for me to merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/platform issues related to the platform community
Projects
None yet
8 participants