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

fix: [M3-7204] - Notice fix & improvements #9755

Merged
merged 7 commits into from
Oct 6, 2023

Conversation

abailly-akamai
Copy link
Contributor

@abailly-akamai abailly-akamai commented Oct 4, 2023

Description 📝

This PR fixes an issue where a Notice is included in an error group but should be treated as a "static" error, meaning that is not being picked up by the scroll-to-error util when it shouldn't (those are for "dynamic" errors, usually when submitting a form). This creates two issues, one being scrolling to this error when intended to scroll to a relevant form submission error, two preventing the scroll to error from happening all together if the notice isn't visible on the page (ex: Linode create flow, hidden in the gpu or premium tab):

Screenshot 2023-10-04 at 12 43 09

It is a bug that bothers me much because missing the root password step in this form is quite easy

Additionally, the PR refactors the Notice to align with our coding standards.

Changes 🔄

  • Add a new optional and documented bypassValidation prop to declare a notice as being static and excluded from an error group
  • Remove unused/deprecated props: breakWords, notificationList & flag which are not in used in the codebase and shouldn't
  • Remove animation on the notice. They were already broken for a long time, and we don't need them. Besides, they create a lot of failing assertions if implemented.
  • Re-establish proper padding on important notices (fixing a regression where they became very skinny)
  • Move styles to their own file
  • Add test coverage for Notice
  • Migration to storybook 7

Preview 📷

The changes shown here should be the only visual changes this PR introduces. Otherwise notices should look the same, and feature the same typography & spacing attributes

Before After
Screenshot 2023-10-04 at 13 02 27 Screenshot 2023-10-04 at 13 05 17

How to test 🧪

Linode Create bug (compare with production)

  1. Navigate to /linodes/create
  2. Select a region and a plan
  3. DO NOT input a root password
  4. Scroll all the way down and click on "Create Linode"
  5. Notice
    • in production there is not scroll behavior
    • in this branch you will be taken to the root password error

Storybook

  1. Run yarn storybook and navigate to the new story
  2. Compare and compare styling against our current design system notices
  3. Confirm only intended changes are showing

Overal Styling

  1. Navigate through the Cloud manager and confirm notices are looking as expected

@abailly-akamai abailly-akamai requested a review from a team as a code owner October 4, 2023 16:34
@abailly-akamai abailly-akamai requested review from mjac0bs and carrillo-erik and removed request for a team October 4, 2023 16:34
@abailly-akamai abailly-akamai marked this pull request as draft October 4, 2023 16:34
@abailly-akamai abailly-akamai self-assigned this Oct 4, 2023
export const Notice = (props: NoticeProps) => {
const {
breakWords,
bypassValidation = false,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the new prop. Only applied in one instance as part of this PR (SelectPlan panel for GPU and premium)

@abailly-akamai abailly-akamai marked this pull request as ready for review October 4, 2023 17:24
@carrillo-erik
Copy link
Contributor

Everything looks good to me from UI and functionality, in particular the focus on the root password field on form validation. Left a comment where I believe you missed a typo or forgot to make some minor edit.

Copy link
Contributor

@mjac0bs mjac0bs left a comment

Choose a reason for hiding this comment

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

Lots of great clean up here, including the story revamp! And thanks for the clear testing instructions -- confirmed that the password field, when left blank and form is submitted, does scroll into view with an error. I haven't seen any skinny notices across the app. Also confirmed that breakWords, flag, and notificationList were unused Notice props.

Left a couple small comments, but 🚢 .

@mjac0bs mjac0bs added the Approved Multiple approvals and ready to merge! label Oct 5, 2023
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!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants