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

feat: [M3-6735] - VPC Edit drawer #9528

Merged
merged 11 commits into from
Aug 11, 2023

Conversation

hana-akamai
Copy link
Contributor

@hana-akamai hana-akamai commented Aug 9, 2023

Description 📝

Add the ability to edit a VPC

Preview 📷

Screen.Recording.2023-08-09.at.2.52.57.PM.mov

How to test 🧪

yarn test VPCEditDrawer
yarn test VPCRow

Create a VPC via the API, click on the edit button and change the label/description. Check the table/drawer again to ensure it updated

@hana-akamai hana-akamai added the VPC Relating to VPC project label Aug 9, 2023
@hana-akamai hana-akamai self-assigned this Aug 9, 2023
@bnussman-akamai bnussman-akamai added Add'tl Approval Needed Waiting on another approval! and removed Ready for Review labels Aug 10, 2023
@bnussman-akamai bnussman-akamai added Approved Multiple approvals and ready to merge! and removed Add'tl Approval Needed Waiting on another approval! labels Aug 11, 2023
@jdamore-linode
Copy link
Contributor

I think there are some accessibility and usability issues with the changes involving the tooltip on the disabled select.

  • Users who fill out forms using tab / keyboard navigation cannot focus the Select component, so they never see the tooltip content (screen recording 🎬)
  • This also applies to users using VoiceOver (I can't seem to get VoiceOver to announce the disabled tooltip at all, even when I do have my cursor hovered over the component) (screen recording 🎬)
  • It doesn't appear to work on iOS devices (I'm guessing Android devices would behave similarly) (screen recording 🎬)
    • Separately, the Select component's disabled state seems a lot less obvious on iOS devices

I'm also wondering what the motivations are behind this from a design and user experience point of view. I'm guessing it's easier to lay out the form without accounting for the help icon, and may look cleaner aesthetically, but I don't think removing it results in a better experience for our users -- I think the help icon plays an important role by telegraphing to the user that there is additional contextual information available.

@jdamore-linode
Copy link
Contributor

@hana-linode Added an integration test for the VPC landing page edit flow. It confirms that VPC labels and descriptions can be edited and that the page updates to reflect the changes.

You can run the tests with this command:

yarn cy:run -s "cypress/e2e/core/vpc/vpc-landing-page.spec.ts"

@hana-akamai
Copy link
Contributor Author

@jdamore-linode Talked with Andrew and he said we can keep the help icon in light of the issues you mentioned

@hana-akamai hana-akamai merged commit 3dba7fc into linode:develop Aug 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Approved Multiple approvals and ready to merge! VPC Relating to VPC project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants