Skip to content

change: [M3-7266] - Improve Linode Create Error Placement#9788

Merged
bnussman-akamai merged 2 commits intolinode:developfrom
bnussman-akamai:M3-7266-fix-linode-create-error-placement
Oct 12, 2023
Merged

change: [M3-7266] - Improve Linode Create Error Placement#9788
bnussman-akamai merged 2 commits intolinode:developfrom
bnussman-akamai:M3-7266-fix-linode-create-error-placement

Conversation

@bnussman-akamai
Copy link
Member

@bnussman-akamai bnussman-akamai commented Oct 12, 2023

Description 📝

  • Finally fixes error placement on the Linode Create page 🎉 (This is long overdue)

Changes 🔄

  • Moves Notice style errors below headings
  • Uses Text Field errorText for errors that relate to TextFields

Preview 📷

Before After
Screenshot 2023-10-12 at 11 25 34 AM Screenshot 2023-10-12 at 11 25 44 AM
Screenshot 2023-10-12 at 11 22 24 AM Screenshot 2023-10-12 at 11 25 03 AM
Screenshot 2023-10-12 at 11 43 49 AM Screenshot 2023-10-12 at 11 43 15 AM
Screenshot 2023-10-12 at 11 48 52 AM Screenshot 2023-10-12 at 11 47 53 AM

How to test 🧪

Verification steps

  • Go to http://localhost:3000/linodes/create
  • Do not fill out the form
  • Click Create Linode at the bottom
  • Check the styles of all validation errors ⛔️
  • Check general spacing of the Linode Create page 📏
  • We should also verify the Linode Rebuild Dialog looks ok because it shares code with the Linode Create flow 🏗️
  • We should also verify Marketplace App user defined passwords looks ok because it shares code with the Linode Create flow 🔐

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

@bnussman-akamai bnussman-akamai added Enhancement UX/UI Changes for UI/UX to review labels Oct 12, 2023
@bnussman-akamai bnussman-akamai self-assigned this Oct 12, 2023
return (
<Paper
sx={{
backgroundColor: theme.color.white,
Copy link
Member Author

Choose a reason for hiding this comment

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

This made the paper the wrong color

>
<div className={innerClass}>
{error && <Notice variant="error">{error}</Notice>}
<Grid container sx={{ display: 'flex' }}>
Copy link
Member Author

Choose a reason for hiding this comment

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

Great example of why a simple Box with flex is much cleaner than a Grid

boxShadow: `inset 0 -1px 0 ${theme.borderColors.divider}`,
marginBottom: theme.spacing(3),
marginTop: 22,
marginTop: theme.spacing(1),
Copy link
Member Author

Choose a reason for hiding this comment

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

This tightens up the spacing between the "Linode Plan" heading and the plan tabs

className
)}
>
{error ? <Notice text={error} variant="error" /> : null}
Copy link
Member Author

Choose a reason for hiding this comment

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

This error is now passed to errorText of the PasswordInput

sx={{ flexGrow: 1, ...sx }}
>
<div className={innerClass}>
{error && <Notice variant="error">{error}</Notice>}
Copy link
Member Author

Choose a reason for hiding this comment

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

This error is now below the heading

@bnussman-akamai bnussman-akamai marked this pull request as ready for review October 12, 2023 18:22
@bnussman-akamai bnussman-akamai requested a review from a team as a code owner October 12, 2023 18:22
@bnussman-akamai bnussman-akamai requested review from cliu-akamai and dwiley-akamai and removed request for a team October 12, 2023 18:22
@bnussman-akamai
Copy link
Member Author

Should this be a change or fix? The before is so egregious fix might be warrented

@bnussman-akamai bnussman-akamai changed the title fix: [M3-7266] - Improve Linode Create Error Placement change: [M3-7266] - Improve Linode Create Error Placement Oct 12, 2023
@mjac0bs
Copy link
Contributor

mjac0bs commented Oct 12, 2023

Should this be a change or fix? The before is so egregious fix might be warrented

I'd vote for fix. There was a lot of 🧹 🧼 here!

Copy link
Contributor

@jaalah-akamai jaalah-akamai left a comment

Choose a reason for hiding this comment

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

In your screenshot of the marketplace app Minecraft: Java Edition I noticed we're using grid items without a grid container. See https://github.com/linode/manager/blob/develop/packages/manager/src/features/StackScripts/UserDefinedFieldsPanel/UserDefinedFieldsPanel.tsx#L252
We probably want to do something like:

<Grid container key={`${field}-${idx}`} spacing={2}>
    {renderField(udf_data, handleChange, field, error)}
</Grid>

import { Tabs } from 'src/components/ReachTabs';
import { Typography } from 'src/components/Typography';

import { Box } from '../Box';
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to do src/components/Box?

@jaalah-akamai
Copy link
Contributor

✅ Tested all the screens and errors. Approving since my suggestions were minor

@bnussman-akamai bnussman-akamai added the Add'tl Approval Needed Waiting on another approval! label Oct 12, 2023
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.

Styles of all validation errors ✅
General spacing of the Linode Create page ✅
Linode Rebuild Dialog looks ok ✅
Marketplace App user defined passwords look ok ✅

@mjac0bs mjac0bs added Approved Multiple approvals and ready to merge! and removed Add'tl Approval Needed Waiting on another approval! labels Oct 12, 2023
@bnussman-akamai bnussman-akamai merged commit 5e023c7 into linode:develop Oct 12, 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! Enhancement UX/UI Changes for UI/UX to review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants