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-7284, M3-7361] - Create VPC drawer in Linode Create flow; Update widths for Create VPC and Create Firewall buttons #9847

Merged
merged 45 commits into from
Nov 7, 2023

Conversation

coliu-akamai
Copy link
Contributor

@coliu-akamai coliu-akamai commented Oct 27, 2023

Description 📝

  • Add Create VPC drawer to the Linode Create flow. Now, if a user wants to create a VPC while creating a linode, they can do so while staying on the same page. No more new tabs!
  • Update 11/6: including M3-7361 in this ticket (as it'd been caught and handled here earlier anyway)

Changes 🔄

List any change relevant to the reviewer.

  • Add CreateVPCDrawer
  • Abstracted a lot of common logic between VPCCreate page and VPCCreateDrawer, creating a new custom hook and other components
  • Added some tests for new components
  • Created new constants in the VPC's constants file for some text
  • some VPC Create page cosmetic changes - Right aligned 'Create VPC' button on VPC create page, removed divider between last subnet and 'Add another subnet' button

Preview 📷

Include a screenshot or screen recording of the change
(the grey shadow in the second img is from my computer's menu bar)

Region that doesn't support vpcs selected:
image

drawer img 1 drawer img 2
image image

How to test 🧪

Prerequisites

  • Using the dev environment, with vpc admin tags

Verification steps

On the Linode Create page

  • Verify that if you do not have a region selected, or have a region that does not support VPCs selected, the Create VPC does not appear and there's instead text that says "VPC is not available in the selected region."
  • If the Create VPC link shows up:
    • Verify that the region you've chosen is pre-selected and the region drop down is disabled
    • Verify VPC Create functionality (it should work the same as the VPC Create page EXCEPT that you must have at least 1 subnet in this flow)
    • Verify that after creating a VPC, the VPC is selected in the VPC panel and that creating a linode with that VPC works as expected

On the VPC Create page

  • verify the 'Create VPC' button is now right aligned, not left aligned
  • verify there's no divider between the last subnet and the 'Add another subnet' button (and that there's increased spacing between the last subnet and add subnet button)
  • verify that besides those changes ^, the VPC Create page looks the same
  • verify that the VPC Create page still functions the same

On the VPC details page

  • verify the Create Subnet drawer still looks the same (and still functions the same, although its functionality shouldn't have been affected by these changes)

Tests

  • yarn test VPC to run all the VPC related tests
  • yarn cy:run -s 'cypress/e2e/core/vpc/vpc-create.spec.ts to make sure there are no regressions here

As an Author I have considered 🤔

Check all that apply

  • 👀 Doing a self review
  • ❔ Our contribution guidelines
  • 🤏 Splitting feature into small PRs
  • ➕ Adding a changeset
  • 🧪 Providing/Improving test coverage
  • 🔐 Removing all sensitive information from the code and PR description
  • 🚩 Using a feature flag to protect the release
  • 👣 Providing comprehensive reproduction steps
  • 📑 Providing or updating our documentation
  • 🕛 Scheduling a pair reviewing session
  • 📱 Providing mobile support
  • ♿ Providing accessibility support

@coliu-akamai coliu-akamai added the VPC Relating to VPC project label Oct 27, 2023
@coliu-akamai coliu-akamai self-assigned this Oct 27, 2023
Copy link
Contributor Author

@coliu-akamai coliu-akamai left a comment

Choose a reason for hiding this comment

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

Since we disable the Create VPC link in the vpc panel if the linode's region isn't selected or if the region selected doesn't support VPCs, sometimes the link just looks like text (below). Speaking with Andrew about potential friendlier UX options!

Update: resolved, this is the new UX (we hide the Create VPC link)
image

handleSelectVPC?: (vpcId: number) => void;
onDrawerClose?: () => void;
pushToVPCPage?: boolean;
selectedRegion?: string;
Copy link
Contributor Author

@coliu-akamai coliu-akamai Oct 27, 2023

Choose a reason for hiding this comment

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

created custom hook for shared form logic between VPCCreate and VPCCreateDrawer. There are some slight differences in the functionality of the drawer and page though:

  • the page will push to the newly created vpc's detail page, but the drawer only makes the newly created vpc the selected vpc for linode-create flow and closes the drawer
  • the drawer's region is pre-selected, based on the selected linode region

The main changes from the old create vpc logic are at lines 80, 133-139 and 185.

Copy link
Contributor

Choose a reason for hiding this comment

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

I might be missing something -- isn't L185 here just description: '', ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah I might have made some changes to this file that changed the lines - the main difference is for the region initial value, it's now region: selectedRegion ?? '', instead of region: '',

@@ -0,0 +1,15 @@
import * as React from 'react';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Similar to the new custom hook, consolidated shared components for the form itself into this new folder. (added tests for them as well altho they're pretty repetitive since SubnetNode, MultipleSubnetInput, and VPCCreate already tested how they render).

However, because there are some styling differences between the VPCCreateDrawer and VPCCreate page, I did end up having to pass an isDrawer prop down ;-; (renamed specifically to isVPCCreateDrawer for the SubnetNode component, since the Create Subnet drawer also used a SubnetNode, not just the VPCCreateDrawer)

@@ -17,6 +18,7 @@ interface Props {
// extra props enable SubnetNode to be an independent component or be part of MultipleSubnetInput
// potential refactor - isRemoveable, and subnetIdx & remove in onChange prop
idx?: number;
isCreateVPCDrawer?: boolean;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

See comment on CannotCreateVPCNotice for reasoning behind this prop

Comment on lines +64 to +66
const showRemoveButton = isCreateVPCDrawer
? idx !== 0 && isRemovable
: isRemovable;
Copy link
Contributor Author

@coliu-akamai coliu-akamai Oct 27, 2023

Choose a reason for hiding this comment

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

Added this logic bc for the VPCCreateDrawer, we do not want the remove button to show for the first subnet

@coliu-akamai
Copy link
Contributor Author

@dwiley-akamai spoke with Andrew -- we'll be hiding the text/Create drawer link if no region is selected. The VPC panel will look like this in that case:
image

1 - Still having trouble trying to get the description field's height match the mockup. Continuing to look into it 😅
2 - 'Create VPC' link should no longer take up the full width of the panel. I saw that the Create Firewall link had similar behavior -- both should now be fixed 🎉

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.

Still need to finish combing through the code, but:

All tests pass ✅
Linode Create page ✅

  • No region selected --> "Create VPC" does not appear ✅
  • Region selected where VPC is not enabled --> text displayed that says "VPC is not available in the selected region." ✅
  • "Create VPC" link shows up with chosen region pre-selected, Region dropdown is disabled ✅
    • VPC Create drawer functionality ✅
  • VPC created via drawer, newly-created VPC is selected in the VPC panel, Linode creation works as expected ✅

VPC Create page ✅

  • "Create VPC" button is right-aligned ✅
  • No divider between the last subnet and the "Add Another Subnet" button (and that there's increased spacing between the last subnet and add subnet button) ✅

VPC Details --> Create Subnet drawer ✅

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.

Confirmed the UX issues I mentioned last time have been resolved ✅

packages/manager/src/hooks/useCreateVPC.ts Outdated Show resolved Hide resolved
handleSelectVPC?: (vpcId: number) => void;
onDrawerClose?: () => void;
pushToVPCPage?: boolean;
selectedRegion?: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

I might be missing something -- isn't L185 here just description: '', ?

@coliu-akamai coliu-akamai added Add'tl Approval Needed Waiting on another approval! and removed Ready for Review labels Nov 2, 2023
@coliu-akamai coliu-akamai changed the title feat: [M3-7284] - Create VPC drawer in Linode Create flow feat: [M3-7284, M3-7361] - Create VPC drawer in Linode Create flow; Update widths for Create VPC and Create Firewall buttons Nov 6, 2023
coliu-akamai and others added 2 commits November 6, 2023 14:08
… ending (linode#9876)

* use utc for unit test

* more strict assertion

* support est and edt

* fix comment

* improve mocks and tests

---------

Co-authored-by: Banks Nussman <banks@nussman.us>
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.

Functionality LGTM. Since the Create VPC flow is now on the same page, I don't think we need the alwaysrefetch logic anymore
https://github.com/linode/manager/blob/develop/packages/manager/src/queries/vpcs.ts#L35

Screen.Recording.2023-11-06.at.5.24.54.PM.mov

@coliu-akamai
Copy link
Contributor Author

@hana-linode removed the refetch logic, thanks for catching!

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.

On the Linode Create page

  • ✅ Verify that if you do not have a region selected, or have a region that does not support VPCs selected, the Create VPC does not appear and there's instead text that says "VPC is not available in the selected region."
  • If the Create VPC link shows up:
    • ✅ Verify that the region you've chosen is pre-selected and the region drop down is disabled
    • ✅ Verify VPC Create functionality (it should work the same as the VPC Create page EXCEPT that you must have at least 1 subnet in this flow)
    • ✅ Verify that after creating a VPC, the VPC is selected in the VPC panel and that creating a linode with that VPC works as expected
  • ✅ VPCs are no longer refetched on window refocus

On the VPC Create page

  • ✅ verify the 'Create VPC' button is now right aligned, not left aligned
  • ✅ verify there's no divider between the last subnet and the 'Add another subnet' button (and that there's increased spacing between the last subnet and add subnet button)
  • ✅ verify that besides those changes ^, the VPC Create page looks the same
  • ✅ verify that the VPC Create page still functions the same

On the VPC details page

  • ✅ verify the Create Subnet drawer still looks the same (and still functions the same, although its functionality shouldn't have been affected by these changes)

@hana-akamai hana-akamai added Approved Multiple approvals and ready to merge! and removed Add'tl Approval Needed Waiting on another approval! labels Nov 7, 2023
@coliu-akamai coliu-akamai merged commit 8b7cb3a into linode:develop Nov 7, 2023
13 checks passed
@coliu-akamai coliu-akamai deleted the feat-m3-7284-hmm branch November 9, 2023 16:08
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