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.

Sign up for GitHub

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

Jump to bottom

feat: connection name can be edited through UI #12803

Merged
merged 10 commits into from
May 31, 2022
Merged

feat: connection name can be edited through UI #12803

merged 10 commits into from
May 31, 2022

Conversation

harshithmullapudi
Copy link
Contributor

@harshithmullapudi harshithmullapudi commented May 12, 2022 •
edited
Loading

What

closes #4407

Screenshot 2022-05-18 at 2 43 34 PM

Screen.Recording.2022-05-18.at.2.35.16.PM.mp4

Added connection name on the connection listing page

Screenshot 2022-05-18 at 2 45 17 PM

You can give a name to connection while creating one

Screenshot 2022-05-18 at 3 19 29 PM

@harshithmullapudi harshithmullapudi requested a review from a team as a code owner May 12, 2022 07:24
@github-actions github-actions bot added area/frontend area/platform issues related to the platform labels May 12, 2022
@harshithmullapudi harshithmullapudi mentioned this pull request May 12, 2022
Closed
edmundito
edmundito reviewed May 12, 2022
View reviewed changes
Copy link
Contributor

@edmundito edmundito left a comment

Choose a reason for hiding this comment

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

Looking good so far. Requesting a few minor changes.

Please update the description of the PR to include a screenshot and what the change this PR is making regardless of what the issues says.

@harshithmullapudi
Copy link
Contributor Author

@edmundito do you think it makes sense to create a new PR for the last changes that are needed? Or should I continue over this PR?

As this PR gives the essential feature and the latter is more of UX changes I think a new PR is good. What say?

@harshithmullapudi harshithmullapudi force-pushed the harshith/add-connection-name branch from fa327a2 to 164202b Compare May 18, 2022 09:09
@harshithmullapudi
Copy link
Contributor Author

@edmundito Made the requested changes

  1. Added connection name in the connection listing table
  2. Made the design changes

@harshithmullapudi
Copy link
Contributor Author

Also updated the PR description with the screenshots and a short video

@edmundito edmundito requested a review from a team May 18, 2022 16:03
@edmundito
Copy link
Contributor

Tested locally, I found some issues with the edit name with long names:
image
Becomes:
image

Additionally, there should be a save button for accessibility reasons.

Testing long names, should that be centered? Where should the alpha tag go?
image

edmundito
edmundito reviewed May 19, 2022
View reviewed changes
Copy link
Contributor

@edmundito edmundito left a comment

Choose a reason for hiding this comment

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

Code looks good so far, I tested and pointed out some issues I saw (see previous comment). The end to end tests may need to be updated to set and confirm the connection name.

@edmundito edmundito requested a review from timroes May 19, 2022 14:05
@harshithmullapudi harshithmullapudi force-pushed the harshith/add-connection-name branch from 164202b to fc32ef1 Compare May 23, 2022 14:34
@harshithmullapudi
Copy link
Contributor Author

harshithmullapudi commented May 23, 2022 •
edited
Loading

Screenshot 2022-05-23 at 7 45 28 PM

Screenshot 2022-05-23 at 7 46 08 PM

Changed a bit of UI. I moved the ReleaseStage near the source connector name

@harshithmullapudi
Copy link
Contributor Author

harshithmullapudi commented May 23, 2022 •
edited
Loading

Screenshot 2022-05-23 at 8 07 04 PM

Made the Input 100% so it can bear the name for a certain length

@harshithmullapudi
Copy link
Contributor Author

@edmundito and @timroes can you review this when you folks get time

edmundito
edmundito suggested changes May 24, 2022
View reviewed changes
Copy link
Contributor

@edmundito edmundito left a comment

Choose a reason for hiding this comment

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

@harshithmullapudi tested the visual changes locally and it looks good. I just added some specific feedback due to gotchas we're learning recently about editing connections. Also, looks like an e2e test is still failing.

airbyte-webapp/src/pages/ConnectionPage/pages/ConnectionItemPage/components/ConnectionName.tsx Outdated
prefix: connection.prefix,
};

await updateConnection({
Copy link
Contributor

Choose a reason for hiding this comment

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

@timroes Here's a situation that concerns me in that, to update the connection name, we need to dump the entire state of the connection because we cannot request a partial update. If the connection model changes in the future we also have to update this code. I think this is ok as a temporary measure and in an effort to get the work on the PR complete, but it may make sense to request an api call that only updates the connection name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah this will be helpful either way

  1. A new API call
  2. The API takes only which needs to be updated

@edmundito
Copy link
Contributor

edmundito commented May 24, 2022 •
edited
Loading

Just one more note about the styling. Maybe some of the margin needs to be adjusted so that when the user hovers it does not overlap the arrow so closely.
image

Here's what it looks like in the prototype:
image

image

Looks like the margins are around left 20, right 15 on each button but please measure in Figma.

While writing this, I noticed that the Postgres logo is also smaller than the Pokemon one and "enabled" is not all caps.

@harshithmullapudi
Copy link
Contributor Author

Screenshot 2022-05-24 at 6 10 35 PM

Screenshot 2022-05-24 at 6 11 14 PM

Screenshot 2022-05-24 at 6 12 26 PM

@edmundito thanks for the feedback. I have made the respective changes for the UI. I think it looks much cleaner now

@harshithmullapudi
Copy link
Contributor Author

harshithmullapudi commented May 24, 2022 •
edited
Loading

@edmundito I also fixed e2e tests.

@edmundito
Copy link
Contributor

I tested the changes and it's looking really good. One more thing I noticed is that I have to click twice to edit the connection name:

  1. Click on the connection name, goes into edit mode
  2. Try typing - nothing!
  3. Click again. Now it's focused on the edit

Another case - "cancel" editing

  1. Click on the connection name, goes into edit mode
  2. Click outside the connection name, stays in connection mode
  3. Click on the connection name again, now input is focused
  4. Click outside the connection name input, now it goes back to non-edit mode

@github-actions github-actions bot added the area/frontend Related to the Airbyte webapp label May 24, 2022
@harshithmullapudi
Copy link
Contributor Author

@edmundito Makes sense. I added a new prop for input called defaultFocus when passed will focus on the input so to avoid this problem and also can help future developments.

wdyt?

edmundito
edmundito reviewed May 25, 2022
View reviewed changes
Copy link
Contributor

@edmundito edmundito left a comment

Choose a reason for hiding this comment

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

Changes look good, but I found one more thing during testing...

@harshithmullapudi
Copy link
Contributor Author

@edmundito do you think it makes sense to have a variable loading for input to add loader prefix/suffix the input component?

@harshithmullapudi
Copy link
Contributor Author

Anyways I have fixed it by adding a state and disabling the input but ideally, maybe we can move back to having a button on the right side and using formik with it.

@harshithmullapudi harshithmullapudi force-pushed the harshith/add-connection-name branch from d3fa8ad to 6ae5a08 Compare May 27, 2022 17:18
@harshithmullapudi harshithmullapudi merged commit ae2aea8 into master May 31, 2022
@harshithmullapudi harshithmullapudi deleted the harshith/add-connection-name branch May 31, 2022 05:19
@octavia-squidington-iii octavia-squidington-iii mentioned this pull request May 31, 2022
Merged
jscottpolevault pushed a commit to jscottpolevault/airbyte that referenced this pull request Jun 1, 2022
@harshithmullapudi
feat: connection name can be edited through UI (airbytehq#12803)
44f54a9 
* feat: connection name can be edited through UI

* fix: changes requested by edmundito

* 1. feat: Added connection name edit in connection settings
2. feat: Added connection name in listing connections page
3. fix: requested changes

* chore: handle UI and e2e tests

* chore: fix e2e tests for webapp

* fix: UI margins

* fix: webe2e tests

* fix: input is not focused when clicked on name

* fix: disable input while making API call

* feat: added enter/esc functionality to connection name edit
@edmundito edmundito mentioned this pull request Jun 6, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@krishnaglick krishnaglick krishnaglick left review comments

@edmundito edmundito edmundito approved these changes

@timroes timroes Awaiting requested review from timroes

Assignees
No one assigned
Labels
area/frontend Related to the Airbyte webapp area/platform issues related to the platform
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Allow setting connection name in the UI
3 participants
@harshithmullapudi @edmundito @krishnaglick