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: [M3-6743] - Create Subnet Drawer #9652

Merged
merged 16 commits into from
Sep 13, 2023
Merged

feat: [M3-6743] - Create Subnet Drawer #9652

merged 16 commits into from
Sep 13, 2023

Conversation

coliu-akamai
Copy link
Contributor

@coliu-akamai coliu-akamai commented Sep 8, 2023 •
edited
Loading

Description 📝

  • Add a create subnet drawer

Preview 📷

Screen.Recording.2023-09-08.at.10.37.49.AM.mov

How to test 🧪

yarn test CreateSubnetDrawer
yarn test SubnetNode
yarn cy:run -s "cypress/e2e/core/vpc/vpc-details-page.spec.ts"

  • On your dev account, navigate to a VPC's detail page and ensure that you can create a subnet / that appropriate errors show up
  • make sure there are no visual changes to VPCCreate (which also uses SubnetNode)

@coliu-akamai coliu-akamai added the VPC Relating to VPC project label Sep 8, 2023
@coliu-akamai coliu-akamai self-assigned this Sep 8, 2023
coliu-akamai
coliu-akamai commented Sep 8, 2023
View reviewed changes
packages/manager/src/utilities/subnets.ts
@@ -1,6 +1,6 @@
import { determineIPType, vpcsValidateIP } from '@linode/validation';

export const DEFAULT_SUBNET_IPV4_VALUE = '10.0.0.0/24';
export const DEFAULT_SUBNET_IPV4_VALUE = '10.0.4.0/24';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

UI mocks updated to use 10.0.4.0/24 as default subnet ip value, so changed it here (+ had to update a lot of related tests)

packages/validation/src/vpcs.schema.ts
@@ -1,7 +1,7 @@
import ipaddr from 'ipaddr.js';
import { array, lazy, object, string } from 'yup';

const LABEL_MESSAGE = 'VPC label must be between 1 and 64 characters.';
const LABEL_MESSAGE = 'Label must be between 1 and 64 characters.';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If a subnet label was 0 or > 64 characters, this message showed up, so removed VPC from the error message

packages/manager/src/features/VPCs/VPCDetail/SubnetCreateDrawer.tsx
Comment on lines +47 to +74
const newErrors = getErrorMap(['label', 'ipv4'], errors);
setErrorMap(newErrors);
setValues({
label: values.label,
labelError: newErrors.label,
ip: {
...values.ip,
ipv4Error: newErrors.ipv4,
},
});
}
};

const { dirty, handleSubmit, resetForm, setValues, values } = useFormik({
enableReinitialize: true,
initialValues: {
// TODO VPC - add IPv6 when that is supported
label: '',
ip: {
ipv4: DEFAULT_SUBNET_IPV4_VALUE,
availIPv4s: 256,
},
} as SubnetFieldState,
onSubmit: onCreateSubnet,
validateOnBlur: false,
validateOnChange: false,
validationSchema: createSubnetSchema,
});
Copy link
Contributor Author

@coliu-akamai coliu-akamai Sep 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I have a version where I don't use the SubnetNode on one of my branches -- the code for error handling is a bit cleaner (+ we can keep the formik in the same shape as the CreateSubnetPayload) in that case in exchange for a more cluttered form (but both versions should have the same functionality, if SubnetNode is as flexible as I'd originally intended it to be way back during VPCCreate).

@coliu-akamai coliu-akamai marked this pull request as ready for review September 8, 2023 15:49
hana-akamai
hana-akamai reviewed Sep 11, 2023
View reviewed changes
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.

Looks like the inputs are still enabled for restricted users

Screen.Recording.2023-09-11.at.3.50.16.PM.mov

packages/manager/src/features/VPCs/VPCDetail/VPCSubnetsTable.tsx Outdated Show resolved Hide resolved
coliu-akamai and others added 2 commits September 11, 2023 16:01
@coliu-akamai
Copy link
Contributor Author

@hana-linode thanks for catching, should be fixed now :D

@hana-akamai hana-akamai added Add'tl Approval Needed Waiting on another approval! and removed Ready for Review labels Sep 11, 2023
dwiley-akamai
dwiley-akamai reviewed Sep 11, 2023
View reviewed changes
packages/manager/src/features/VPCs/VPCDetail/SubnetCreateDrawer.tsx
ip: {
...values.ip,
ipv4Error: newErrors.ipv4,
},
Copy link
Contributor

Choose a reason for hiding this comment

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

When we get a general error (would end up being stored on errorMap.none since it doesn't belong to a particular field -- you can try blocking network requests to *subnet* to replicate this), it's persisting between drawer openings and closings currently.

Might have to do something like setErrorMap({}) in the useEffect(), although I'm not sure why resetForm() isn't taking care of this aspect

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks for catching, just pushed! Maybe it could be because I've a separate useState hook for the error map due to the SubnetNode, and Formik can't reset that with resetForm? Not too sure though, just a guess 💭

dwiley-akamai
dwiley-akamai approved these changes Sep 12, 2023
View reviewed changes
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.

🎉

packages/manager/src/features/VPCs/VPCCreate/SubnetNode.test.tsx Outdated Show resolved Hide resolved
packages/manager/src/features/VPCs/VPCCreate/SubnetNode.test.tsx Outdated Show resolved Hide resolved
packages/manager/src/features/VPCs/VPCCreate/SubnetNode.test.tsx Outdated Show resolved Hide resolved
packages/manager/src/features/VPCs/VPCCreate/SubnetNode.test.tsx Outdated Show resolved Hide resolved
@coliu-akamai coliu-akamai added Approved Multiple approvals and ready to merge! and removed Add'tl Approval Needed Waiting on another approval! labels Sep 12, 2023
coliu-akamai
coliu-akamai commented Sep 13, 2023
View reviewed changes
...ager/src/features/Longview/LongviewDetail/DetailTabs/ListeningServices/ListeningServices.tsx
@@ -27,10 +27,7 @@ export const ListeningServices = (props: TableProps) => {
const { services, servicesError, servicesLoading } = props;
Copy link
Contributor Author

@coliu-akamai coliu-akamai Sep 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

how did this sneak in? 🤣

maybe an eslint thing after merging

@coliu-akamai coliu-akamai merged commit 353278c into linode:develop Sep 13, 2023
13 checks passed
This was referenced Sep 15, 2023
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dwiley-akamai dwiley-akamai dwiley-akamai approved these changes

@hana-akamai hana-akamai hana-akamai approved these changes

Assignees

@coliu-akamai coliu-akamai

Labels
Approved Multiple approvals and ready to merge! VPC Relating to VPC project
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@coliu-akamai @dwiley-akamai @hana-akamai