Skip to content

feat: [M3-7120] - Create VPC Feedback #9718

Merged
coliu-akamai merged 4 commits intolinode:developfrom
coliu-akamai:feat-m3-7120
Sep 27, 2023
Merged

feat: [M3-7120] - Create VPC Feedback #9718
coliu-akamai merged 4 commits intolinode:developfrom
coliu-akamai:feat-m3-7120

Conversation

@coliu-akamai
Copy link
Contributor

@coliu-akamai coliu-akamai commented Sep 25, 2023

Description 📝

Address VPC Create page related feedback from product

Major Changes 🔄

  • scroll into error for general subnet errors
  • better readability for available IPs #
  • hold off on displaying # of available IPs for non RFC1918 IPs

Preview 📷

Commas for large numbers

Before After
image image

Number of available ips doesn't show up for a non RFC1918 ip

Before After
image image

scroll into view subnet errors
https://github.com/linode/manager/assets/139280159/e3d5218a-2f53-4bc6-8472-7bd08d6a6c34

How to test 🧪

How to verify changes?
You can use the MSW (with VPC feature flag) for the following:

  • On the Create VPC page (as well as the Create Subnet drawer on a VPC's detail page), confirm the following:
    • The # of available IPs message will only show up for RFC1918 IPs.
      • For 10.x.x.x IPs, the mask must be >= 8
      • For 172.16.x.x-172.31.x.x, the mask must be >= 12
      • For 192.168.x.x, the mask must be >= 16
    • If a number shows up and is larger than 999, commas appear

You will need to be on the dev environment with VPC tags on your account for the following:

  • Navigate to the VPC Create page. Verify that if general subnet errors pop up, the page scrolls to them.
    • To produce some of these errors: Input a valid region (currently I believe only Newark works) and VPC label. Then, in the subnets section, add 3-4+ subnets (enough that you can no longer see the first subnet) and give them all the same label and IP. Click Create VPC, and you should see that your screen scrolls to the errors that have appeared.

How to run Unit or E2E tests?
yarn test subnets.test.ts

@coliu-akamai coliu-akamai self-assigned this Sep 25, 2023
@coliu-akamai coliu-akamai added the VPC Relating to VPC project label Sep 25, 2023
@coliu-akamai coliu-akamai marked this pull request as ready for review September 26, 2023 14:46
@coliu-akamai coliu-akamai requested a review from a team as a code owner September 26, 2023 14:46
@coliu-akamai coliu-akamai requested review from jaalah-akamai and tyler-akamai and removed request for a team September 26, 2023 14:46
Copy link
Contributor

@dwiley-akamai dwiley-akamai left a comment

Choose a reason for hiding this comment

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

🚀

Unit tests pass ✅
Window scrolls to general subnet errors towards the top of the Subnets section ✅
Comma-separated large available IP numbers ✅
Available IPs display when expected for RFC1918 IPs ✅

@coliu-akamai coliu-akamai added Add'tl Approval Needed Waiting on another approval! and removed Ready for Review labels Sep 26, 2023
@tyler-akamai
Copy link
Contributor

tyler-akamai commented Sep 26, 2023

Even after filling in the Region and VPC label, it still shows them as an error and scrolls to them.

Screen.Recording.2023-09-26.at.2.17.25.PM.mov

@hana-akamai
Copy link
Contributor

@tyler-akamai We handle errors similarly in other places in the app. Might be a convo for cafe?

Copy link
Contributor

@hana-akamai hana-akamai left a comment

Choose a reason for hiding this comment

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

Confirmed that the unit test passed, numbers are comma-separated, window scrolls to errors, and available IPs display for RFC1918 IPs 🚢

@hana-akamai hana-akamai added Approved Multiple approvals and ready to merge! and removed Add'tl Approval Needed Waiting on another approval! labels Sep 26, 2023
@coliu-akamai
Copy link
Contributor Author

@tyler-akamai @hana-linode thanks for bringing this up! Agreed that this would be a good topic for cafe.

Tried something that gets rid of the errors after we change the fields -- It's currently on my branch feat-m3-7120-offshoot, but I can merge it into here, if we want to go that route (otherwise I'll hold off until frontend cafe confirms this is the behavior we want)

@tyler-akamai
Copy link
Contributor

tyler-akamai commented Sep 26, 2023

  • unit test passed
  • numbers are comma-separated
  • window scrolls to errors
  • available IPs display for RFC1918 IPs

Great job

@coliu-akamai
Copy link
Contributor Author

Merging as integration tests issues were not due to VPC related tests. Leaving the branch feat-m3-7120-offshoot up for if we decide to change the behavior of how form errors are handled

@coliu-akamai coliu-akamai merged commit 8526acc into linode:develop Sep 27, 2023
@coliu-akamai coliu-akamai deleted the feat-m3-7120 branch September 27, 2023 13:35
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.

4 participants