fix: [M3-7170] - Create VPC Region and Label Validation #9750

coliu-akamai · 2023-10-03T19:56:43Z

Description 📝

Ensures that VPC region, label, and description validation error messages do not persist for VPC create flow.

Preview 📷

Screen.Recording.2023-10-04.at.5.39.30.PM.mov

How to test 🧪

How to setup test environment?

With VPC flags on - on the dev environment or on the prod environment (with MSW): navigate to the Create VPC page.

How to verify changes?

Without inputting anything, click on the Create VPC button. You should see errors pop up for the Region and VPC Label fields (as well as the subnet label field, but that's not part of this PR)
Verify that when you select a region, the error disappears.
Verify that when you input a label, the error disappears.
** To test description error messages, you'll need to input a description > 255 characters
Verify that after initial scrolling to errors, the page does not continue to scroll if there are still errors on the page

…scription on VPC Create flow do not incorrectly persist

packages/manager/src/features/VPCs/VPCCreate/VPCCreate.tsx

jdamore-linode · 2023-10-04T16:08:06Z

Note: due to the scrollErrorIntoView functionality, the page will jump to wherever there are errors. This may be worth looking into as a separate issue if it's disruptive enough. Video below:

I am finding this to be more disruptive than I initially thought it would be. I take it there's no way for us to revalidate the fields without also triggering the scroll functionality?

There doesn't seem to be a lot of context for this ticket -- I notice that the behavior we're implementing here is not consistent with other create forms in Cloud (for example, if you trigger an error in the Linode Create flow, the inline error messages persist until clicking the "Create Linode" button again). Given the jumping behavior, and the fact that this is not a pattern used elsewhere in the app (to my knowledge), is there any chance we might want to revisit this from a design perspective to ensure that this is really the right approach?

Edit: Oh, just saw M3-6894 😅 Please disregard

dwiley-akamai

scrollErrorIntoView() takes optional arguments, so you could do

scrollErrorIntoView(undefined, { behavior: 'smooth' });

on L190 of VPCCreate.tsx to avoid a jarring jump

packages/manager/.changeset/pr-9750-fixed-1696365803471.md

packages/manager/src/features/VPCs/VPCCreate/VPCCreate.tsx

Co-authored-by: Dajahi Wiley <114682940+dwiley-akamai@users.noreply.github.com>

coliu-akamai · 2023-10-04T17:51:30Z

Thanks for pointing that out, @dwiley-akamai! It looks a lot smoother now:

Screen.Recording.2023-10-04.at.1.48.56.PM.mov

hana-akamai

Is it expected behavior for the page to continue to scroll after the first initial scroll?

Screen.Recording.2023-10-04.at.4.11.34.PM.mov

tyler-akamai · 2023-10-04T20:19:44Z

Is it expected behavior for the page to continue to scroll after the first initial scroll?

Screen.Recording.2023-10-04.at.4.11.34.PM.mov

I am experiencing the same issue.

coliu-akamai · 2023-10-04T20:52:14Z

thanks @hana-linode and @tyler-akamai! Confirmed with Andrew that we should try to get rid of that second scroll, looking into it :D

jdamore-linode

Thanks @coliu-akamai! Confirming that scrolling no longer continues after resolving the first error

coliu-akamai · 2023-10-04T21:30:06Z

packages/manager/src/features/VPCs/VPCCreate/VPCCreate.tsx

@@ -151,6 +151,10 @@ const VPCCreate = () => {
        visualToAPISubnetMapping
      );
      setFieldValue('subnets', subnetsAndErrors);
+
+      if (errors || generalAPIError || generalSubnetErrorsFromAPI) {
+        scrollErrorIntoView();


Removed the { behavior: 'smooth' } tag from commit 95f64ca since we're no longer scrolling to an error after the initial error (+ looking through the codebase, nothing else has this tag)

coliu-akamai · 2023-10-04T21:31:52Z

packages/manager/src/features/VPCs/VPCCreate/VPCCreate.tsx

@@ -185,11 +189,13 @@ const VPCCreate = () => {
    validationSchema: createVPCSchema,
  });

-  React.useEffect(() => {


Moved the scrolling logic into the try-catch block of onCreateVPC and got rid of the useEffect in order to prevent rescrolling after initial scroll. (Note that a lot of other flows have this logic in a useEffect, so there might be potential rescrolling behavior there too!)

I went ahead and reintroduced the useEffect with the conditional logic for all three types of errors. If I removed the values from the dependency array, I didnt experience any of the page scrolling behavior. Just an FYI. I found your final solution much cleaner and don't recommend any further changes.

hana-akamai

Confirmed that errors no longer persist! 🚫

tyler-akamai · 2023-10-05T13:40:57Z

LGTM, great job!

Verified errors pop up for the Region and VPC Label fields (as well as the subnet label field)
Verified that when you select a region, the error disappears.
Verified that when you input a label, the error disappears.
Verified test description error messages
Verified that after initial scrolling to errors, the page does not continue to scroll if there are still errors on the page

coliu-akamai and others added 4 commits September 25, 2023 17:20

m3-7120

ddac87d

Update subnets.ts

ba2cee0

changeset and some comments

240223f

get rid of field errors after we change the field

9898478

coliu-akamai self-assigned this Oct 3, 2023

coliu-akamai added 2 commits October 3, 2023 15:59

Merge branch 'develop' into feat-m3-7120-offshoot

bc21e86

remove unnecessary file

528cbb5

coliu-akamai added the VPC Relating to VPC project label Oct 3, 2023

coliu-akamai marked this pull request as ready for review October 3, 2023 20:40

coliu-akamai requested a review from a team as a code owner October 3, 2023 20:40

coliu-akamai requested review from mjac0bs, carrillo-erik, dwiley-akamai and tyler-akamai and removed request for a team October 3, 2023 20:40

coliu-akamai added the Ready for Review label Oct 3, 2023

coliu-akamai requested a review from hana-akamai October 3, 2023 20:41

Added changeset: Ensure that error messages for region, label, and de…

9d21311

…scription on VPC Create flow do not incorrectly persist

coliu-akamai commented Oct 3, 2023

View reviewed changes

packages/manager/src/features/VPCs/VPCCreate/VPCCreate.tsx Outdated Show resolved Hide resolved

dwiley-akamai reviewed Oct 4, 2023

View reviewed changes

packages/manager/.changeset/pr-9750-fixed-1696365803471.md Outdated Show resolved Hide resolved

packages/manager/src/features/VPCs/VPCCreate/VPCCreate.tsx Outdated Show resolved Hide resolved

coliu-akamai and others added 2 commits October 4, 2023 12:45

Update packages/manager/.changeset/pr-9750-fixed-1696365803471.md

ef22ab2

Co-authored-by: Dajahi Wiley <114682940+dwiley-akamai@users.noreply.github.com>

abstract methods + make scrolling smooth

95f64ca

hana-akamai reviewed Oct 4, 2023

View reviewed changes

remove rescroll after initial scroll

46d336c

jdamore-linode approved these changes Oct 4, 2023

View reviewed changes

coliu-akamai commented Oct 4, 2023

View reviewed changes

simplified logic for scrolling on errors

c9e301d

coliu-akamai added Add'tl Approval Needed Waiting on another approval! and removed Ready for Review labels Oct 4, 2023

hana-akamai approved these changes Oct 4, 2023

View reviewed changes

carrillo-erik approved these changes Oct 5, 2023

View reviewed changes

tyler-akamai approved these changes Oct 5, 2023

View reviewed changes

coliu-akamai merged commit 0b58eb2 into linode:develop Oct 5, 2023
12 of 13 checks passed

coliu-akamai deleted the feat-m3-7120-offshoot branch October 6, 2023 20:12

This was referenced Oct 12, 2023

Release v1.105.0 - release → staging #9789

Merged

Release v1.105.0 - staging → master (#9789) #9792

Closed

Release v1.105.0 - release → staging #9793

Merged

Release v1.105.0 - staging → master (#9789) #9794

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: [M3-7170] - Create VPC Region and Label Validation #9750

fix: [M3-7170] - Create VPC Region and Label Validation #9750

coliu-akamai commented Oct 3, 2023 •

edited

Loading

jdamore-linode commented Oct 4, 2023 •

edited

Loading

dwiley-akamai left a comment

coliu-akamai commented Oct 4, 2023

hana-akamai left a comment

tyler-akamai commented Oct 4, 2023

coliu-akamai commented Oct 4, 2023

jdamore-linode left a comment

coliu-akamai Oct 4, 2023

coliu-akamai Oct 4, 2023 •

edited

Loading

carrillo-erik Oct 5, 2023

hana-akamai left a comment

tyler-akamai commented Oct 5, 2023 •

edited

Loading

fix: [M3-7170] - Create VPC Region and Label Validation #9750

fix: [M3-7170] - Create VPC Region and Label Validation #9750

Conversation

coliu-akamai commented Oct 3, 2023 • edited Loading

Description 📝

Preview 📷

How to test 🧪

jdamore-linode commented Oct 4, 2023 • edited Loading

dwiley-akamai left a comment

Choose a reason for hiding this comment

coliu-akamai commented Oct 4, 2023

hana-akamai left a comment

Choose a reason for hiding this comment

tyler-akamai commented Oct 4, 2023

coliu-akamai commented Oct 4, 2023

jdamore-linode left a comment

Choose a reason for hiding this comment

coliu-akamai Oct 4, 2023

Choose a reason for hiding this comment

coliu-akamai Oct 4, 2023 • edited Loading

Choose a reason for hiding this comment

carrillo-erik Oct 5, 2023

Choose a reason for hiding this comment

hana-akamai left a comment

Choose a reason for hiding this comment

tyler-akamai commented Oct 5, 2023 • edited Loading

coliu-akamai commented Oct 3, 2023 •

edited

Loading

jdamore-linode commented Oct 4, 2023 •

edited

Loading

coliu-akamai Oct 4, 2023 •

edited

Loading

tyler-akamai commented Oct 5, 2023 •

edited

Loading