upcoming: [M3-7924] - Linode Create Refactor - VPC - Part 10 by bnussman-akamai · Pull Request #10354 · linode/manager

bnussman-akamai · 2024-04-05T00:01:14Z

Description 📝

Adds a VPC section to the new Linode Create flow ✨

Changes 🔄

Builds new VPC panel that uses react hook from tooling 🔧
Adds a VPC Select
Adds Create a Linode to the document title to match the existing flow
Improves error handling for VPC IP ranges

Preview 📷

How to test 🧪

Prerequisites

Turn on the Linode Create v2 feature flag 🎏

Verification steps

Test the new VPC selction 🧪
Compare functionality to the existing Linode Create flow 🔍
Test error handling in the VPC section ❌

As an Author I have considered 🤔

…aceSchema`

bnussman-akamai · 2024-04-08T20:39:17Z

+export const VPCSelect = (props: Props) => {
+  const { filter, value, ...rest } = props;
+
+  const { data, isFetching } = useVPCsQuery({}, filter ?? {});


I will come back and update this to use useAllVPCsQuery or an infinite query after #10322 gets merged

bnussman-akamai · 2024-04-08T20:42:38Z

  };

  const formik = useFormik({
+    enableReinitialize: true,


Had to add this so that region updates when the region is changed on the Linode Create flow. This was not an issue with the existing create flow because the drawer was re-mounted when it was opened (causing the VPC create drawer to not animate)

abailly-akamai

I'll spend more time with it in the UI once we get more reviews from folks more familiar than me with the feature.

All together this looks great, concise, well thought & put together

Left some initial comments to look at a couple areas that could be improved

abailly-akamai · 2024-04-09T14:30:21Z

+        ))}
+      </Stack>
+      <Box>
+        <LinkButton onClick={() => append('')}>Add IPv4 Range</LinkButton>


Same here. I guess I have to get form familiar with these hooks but this just looks weird

This adds an empty string to the ip_ranges array on the VPC interface. Appending an empty string item to the array will cause a new empty TextField to render so that the user can add a new IP range.

Yeah, i am getting to understand that part. i think even doing

// Adding an empty string to the array will render to new empty field const newRangeField = '' => append(newRangeField)

or something like that could help with readability

github-actions · 2024-04-09T16:07:45Z

Coverage Report: ✅
Base Coverage: 81.78%
Current Coverage: 81.8%

dwiley-akamai

I'm still testing and combing through the code, but it's looking good overall so far. A few comments:

Points with screenshots

Can we keep the "X" vertically centered in relation to the input field when there's an error? I think this has come up as an issue in other contexts, maybe there's a related/similar ticket in the backlog somewhere
Can we keep both dropdowns the same width to match prod?
Can we make some width adjustments so if you click far away from the Auto-assign & Assign checkboxes, they don't get toggled?

Screen.Recording.2024-04-09.at.5.02.46.PM.mov

There were some concerns during the initial IPv4 range work a couple of months ago about large header text like this (doesn't make the section hierarchy as clear) instead of the more understated one currently used in prod

hkhalil-akamai

Tested and verified parity with existing VPC flow, code review looks clean too -- excellent work as always!

Also, thanks for including comprehensive unit tests!

hkhalil-akamai · 2024-04-09T22:31:08Z

+                <TextField
+                  errorText={fieldState.error?.message}
+                  hideLabel
+                  label={`IP Range ${index}`}
+                  onChange={field.onChange}
+                  placeholder="10.0.0.0/24"
+                  value={field.value}
+                />


The width of this textfield is shorter (166.5px vs 362.41px) than the original. Personally I prefer the shorter width since it more closely matches the expected length of the subnet value but just wanted to point it out.

Yeah, it does look slightly odd. I wasn't able to easily fix it without writing one-off styles, so I'd prefer to leave it be

bnussman-akamai · 2024-04-10T13:50:22Z

I fixed point 1, 3, and 4. @dwiley-akamai

I personally don't agree with 4. I was thinking an h3 would be fine if every other header on the page is an h2. I've made the change regardless.

I'm declaring point 2 out of scope because I told myself I wouldn't write one-off styling in this project 😖 I think the issue would have to be fixed at the Autocomplete component level if we want it to work that way globally.

abailly-akamai

Took a second pass at the UI and found regressions/improvements to be made to the UX. See comments for details

abailly-akamai · 2024-04-10T13:23:49Z

+      <Stack spacing={2}>
+        <Typography variant="h2">VPC</Typography>
+        <Typography>
+          Assign this Linode to an existing VPC.{' '}


Production has "Allow Linode to communicate in an isolated environment." is this intentional?

Fixed in 70d8b1e

Note for UX:

It's a tiny bit unclear for the user you need to select a region to be able to select a VPC. Again, this is parity but something to think about. Weird it never came up 🤷

Agreed. I'll make an effort to circle back around to all of the weird UX things after this refactor happens

abailly-akamai · 2024-04-10T13:39:36Z

+      loading={isFetching}
+      options={data?.data ?? []}
+      value={selectedVPC}
+      {...rest}


We need to reset the VPC panel on region change. Repro:

select an available region (ex: chicago)

select a VPC

change to a non-available region. ex: Newark

see screenshot below

You can basically keep interacting with the whole VPC form as the previous valus is still selected

This is expected for now. I am saving "side effect" state updates for later in this project.

This is a local side effect, not a global one tho. You gotta point those out in your PR description

What I mean is that I haven't determined the best way to handle side-effect state changes using react-hook-form so I'm deferring the work till later.

There needs to be some investigation on my part to find the best way to handle these "side-effect" changes because there are a ton in the Linode Create flow.

Got it. Would still be helpful for reviewers to know what's not supposed to work. I remembered default values being one, not those (tho they are clearly related).

I have a running list so I assure you these will be fixed! I will do my best to call these out in the future

abailly-akamai

Amazing work! this is coming alive 🎉

dwiley-akamai

The core functionality LGTM, nice work as the code patterns are pretty clean 🧽

I do think the remaining visual inconsistencies and regressions have to be addressed, but that can be in a follow-up or at least a circle-back before the Linode Create refactor is wrapped up.

Other quirks with IPv4 ranges

When there's an error, the field it belongs to gets horizontally widened, which makes me favor retaining the wider field width that we currently have in prod.

The vertical distance between the fields is now greatly compressed compared to prod. I think that should be adjusted.

This isn't an issue per se, but I noticed that compared to prod, you now have to input something into the most recent field before clicking "Add IPv4 Range" adds a new input field.

Also, were you going to address the leftover changes from #10342 (comment) in this PR?

dwiley-akamai · 2024-04-10T16:03:26Z

+  const copy =
+    data?.results === 0
+      ? 'Allow Linode to communicate in an isolated environment.'
+      : 'Assign this Linode to an existing VPC.';


minor: We lost some parity here -- a fresh form in prod shows Allow Linode to communicate in an isolated environment. but this branch shows Assign this Linode to an existing VPC.

Good catch. I believe the logic should now match the prod flow. (1934915)

initial work in VPC section

6427449

bnussman-akamai added the Linode Create Refactor label Apr 5, 2024

bnussman-akamai self-assigned this Apr 5, 2024

bnussman added 11 commits April 5, 2024 09:44

prefill region in create VPC drawer

ea70590

Added changeset: Linode Create Refactor - VPC - Part 10

1622cc1

make behavior match production more and add summary item

8aa53a2

add document segment title

80d21f5

start unit testing

e447254

add more unit testing

1b986ef

improve ordering of interfaces and improve error handling

501ab4b

dial in interfaces logic

f87a029

more unit testing

95881e2

improve validation error message

3a5dac9

Added changeset: Improved VPC ip_ranges validation in `LinodeInterf…

d8b7f73

…aceSchema`

bnussman-akamai commented Apr 8, 2024

View reviewed changes

bnussman-akamai marked this pull request as ready for review April 8, 2024 20:40

bnussman-akamai requested a review from a team as a code owner April 8, 2024 20:40

bnussman-akamai requested review from cpathipa and dwiley-akamai and removed request for a team April 8, 2024 20:40

bnussman-akamai added the Ready for Review label Apr 8, 2024

bnussman-akamai commented Apr 8, 2024

View reviewed changes

bnussman-akamai requested review from abailly-akamai, hkhalil-akamai and jaalah-akamai April 8, 2024 20:45

abailly-akamai reviewed Apr 9, 2024

View reviewed changes

bnussman added 2 commits April 9, 2024 11:41

add some comments

6473665

fix spelling

3f159bb

dwiley-akamai reviewed Apr 9, 2024

View reviewed changes

hkhalil-akamai approved these changes Apr 9, 2024

View reviewed changes

bnussman added 3 commits April 10, 2024 09:27

make IP Range X button align with input

3b288d8

fix checkbox clickable area

9ed9a30

use bold text insted of h3 heading

a3715e3

bnussman-akamai requested review from abailly-akamai and dwiley-akamai April 10, 2024 13:53

abailly-akamai requested changes Apr 10, 2024

View reviewed changes

re-add conditional copy

70d8b1e

abailly-akamai approved these changes Apr 10, 2024

View reviewed changes

dwiley-akamai approved these changes Apr 10, 2024

View reviewed changes

bnussman added 4 commits April 10, 2024 12:57

dial in styles @dwiley-akamai

8b47ea0

fix spelling from old PR @dwiley-akamai

f8d374e

fix conditional copy logic

1934915

make vpc select match with of all others on the page

8a14847

bnussman-akamai merged commit df09173 into linode:develop Apr 10, 2024

This was referenced Apr 25, 2024

Release v.118.0 - release → staging #10409

Closed

Release v1.118.0 - release → staging #10410

Merged

Release v1.118.0 - staging → master #10416

Merged

bnussman-akamai mentioned this pull request Nov 12, 2024

fix: sx styling for Textfield component #11246

Merged

12 tasks

Conversation

bnussman-akamai commented Apr 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description 📝

Changes 🔄

Preview 📷

How to test 🧪

Prerequisites

Verification steps

As an Author I have considered 🤔

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

abailly-akamai left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

abailly-akamai Apr 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions Bot commented Apr 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dwiley-akamai left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hkhalil-akamai left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bnussman-akamai commented Apr 10, 2024

Uh oh!

abailly-akamai left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bnussman-akamai Apr 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

abailly-akamai left a comment

Choose a reason for hiding this comment

Uh oh!

dwiley-akamai left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

bnussman-akamai commented Apr 5, 2024 •

edited

Loading

abailly-akamai Apr 9, 2024 •

edited

Loading

github-actions Bot commented Apr 9, 2024 •

edited

Loading

dwiley-akamai left a comment •

edited

Loading

bnussman-akamai Apr 10, 2024 •

edited

Loading

dwiley-akamai left a comment •

edited

Loading

bnussman-akamai Apr 10, 2024 •

edited

Loading