feat: [M3-8069] - scrollErrorIntoViewV2 - POC

[abailly-akamai]

Description 📝

This utility is the version 2 of the scrollErrorIntoView utility (formik forms).
It uses a MutationObserver to solve the issue of the form not always being fully rendered when the scrollErrorIntoView function is called, resulting in the error not being scrolled into view. ex: https://github.com/linode/manager/pull/10429/files.

We are likely to live with formik forms and class components for some more time, so the util is meant to be versatile for both class and functional components, therefore avoiding hooks. The migration from the v1 to the v2 util is pretty straight forward, the only argument being the form container ref via:

• React.createRef() (class components)
• React.useRef() (functional components)

A MutationObserver is not generally expensive in terms of computing. It is designed to be a low-level, primitive API that has minimal impact on performance. Here the MutationObserver is configured to observe a single form element and its subtree for changes to attributes and the child list. The observer also disconnects as soon as it finds an error, further limiting its potential impact on performance and/or memory leaks.

ℹ️ The previous iteration of scrollErrorIntoView provided optional ScrollIntoViewOptions arguments to customize default scroll behavior, but I found out those are never used so they were intentionally left out of the v2 util.

Changes 🔄

List any change relevant to the reviewer.

• Build new scrollErrorIntoViewV2 component
• Implement in issue prone fix: [M3-7977] - Surface interface error in Linode Config dialog  #10429 (for example)
• Mark legacy scrollErrorIntoView as deprecated

Preview 📷

How to test 🧪

Class Component

ℹ️ Where user is a secondary, limited scope user

Reproduction steps

• Navigate to http://localhost:3000/account/users/{user}/permissions
• Block /grants API requests
• Scroll to very bottom of the form and click "Save"
• Notice no scroll to error happens

Verification steps

• Navigate to http://localhost:3000/account/users/{user}/permissions
• Block /grants API requests
• Scroll to very bottom of the form and click "Save"
• Confirm being scrolled to error

Functional Component

Reproduction steps

• Navigate to http://localhost:3000/linodes/54671103/configurations
• Click on "Add Configuration"
• Scroll to bottom of form and click "Add Configuration"
• Notice no scroll to error happens

Verification steps

• Navigate to http://localhost:3000/linodes/54671103/configurations
• Click on "Add Configuration"
• Scroll to bottom of form and click "Add Configuration"
• Confirm being scrolled to error

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

[github-actions]

Coverage Report: ✅
Base Coverage: 81.52%
Current Coverage: 81.55%

[mjac0bs]

Thanks for POCing this solution. Confirmed this fixes the lack of scroll behavior in Linode Config drawers - both field and notice errors look good and the first error scrolls into view if there are multiple.

It would be nice if this PR included an implementation of scrollErrorIntoViewV2 with a class component, but I don't know if there's a simple one. UserPermissions.tsx has several uses of the v1 util where scrolling is currently broken. (One way to repro this is by visiting a restricted user's permissions page, then blocking network requests to their /grants endpoint and submitting the General Permissions form via Save.) But I ended up with forwardRef errors when I briefly attempted to switch out one of the functions and didn't proceed further.

As long as we decide to proceed with this approach:

• after merging I can update fix: [M3-7977] - Surface interface error in Linode Config dialog  #10429 to just fix the unsurfaced errors, unrelated to scroll error into view
• do we want to go with a progressive refactor to the new util, creating tickets as issues come up, and potentially working known places like UserPermissions into a plan for refactoring that page overall?
• in light of this PR and the discussion on the Linode Create Refactor PR, maybe we mention scrollErrorIntoViewV2 in the Error Handling section of our developer docs. We currently mention nothing about how we handle scroll behavior, and since it sounds like we have agreement on the approach to take here (adopting v2 of util, not RHF), it would be a nice reference point for future contributors.

[abailly-akamai]

@mjac0bs Great points - let me address two things as part of this PR

• a class component implementation/fix
• documentation

[abailly-akamai]

@mjac0bs all set and PR description updated ✨

[mjac0bs]

Reapproving after confirming latest changes. I did leave some minor docs nits for readabiliity.

[mjac0bs]

This makes more sense than the existing container where I had tried to put the ref. 👍🏼

Looks great!

Screen.Recording.2024-05-15.at.2.31.49.PM.mov

[hana-akamai]

Confirmed that errors scroll into view 🎉 . It's a bit weird to me that the user permissions success is a toast but the error is a notice. Thoughts on making the error a toast?

[mjac0bs]

the user permissions success is a toast but the error is a notice. Thoughts on making the error a toast?

@hana-linode I see what you mean but think this would be out of scope here and a better thing to consider when looking at refactor of UserPermissions overall, including the UX, which we talked a little bit about last week. (We do have a placeholder epic [M3-7591] for the class to function component refactor, just still TODO.) Also: if it's an error toast, I'd encourage us to make it persistent. And persistent toasts have potential issues at the moment since the cancel button is unreliable.

[abailly-akamai]

Confirmed the test failure was a flake! merging to get ready to implement more of the v2 util in small testable PRs