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

refactor: [M3-6282] MUI v5 Migration - SRC > Features > StackScripts pt2 #9786

Merged
merged 24 commits into from
Oct 18, 2023

Conversation

coliu-akamai
Copy link
Contributor

@coliu-akamai coliu-akamai commented Oct 11, 2023

Description 📝

Part 2 of migrating the MUI styling for the StackScripts package. This PR is for the StackScriptsPanel, UserDefinedFieldsPanel, subpackages and the files in the root package.

Once Part 1 gets merged in, will rebase this PR to get rid of merge conflicts

Changes 🔄

  • Migrated styling from makeStyles to use Styled components and sx
  • Removed React.FC
  • updated imports to be named exports instead of default exports (unless lazily exported / the component had higher order functions applied to it)
  • removed unnecessary index.tsx files

How to test 🧪

There should be no visual changes.

Prerequisites

(How to setup test environment)

  • Set up local host and navigate to the stack scripts section

Verification steps

(How to verify changes)

  • Verify that the StackScripts details page is the same
  • Verify that rebuilding a linode from a StackScript still looks and functions the same

Additionally re-verify part I's checks:

  • Verify that there are no visual changes on the StackScript landing page
  • Verify that the StackScript create page still looks and functions the same
  • Verify that the StackScript section in the Linode Create flow still looks and functions the same

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

@coliu-akamai coliu-akamai marked this pull request as ready for review October 12, 2023 14:22
@coliu-akamai coliu-akamai requested a review from a team as a code owner October 12, 2023 14:22
@coliu-akamai coliu-akamai requested review from mjac0bs and hana-akamai and removed request for a team October 12, 2023 14:22
);
};

export default recompose<CombinedProps, Props & RenderGuardProps>(
RenderGuard,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed this RenderGuard as the StackScriptRow isn't using it (no updateFor props passed in where it's being used)

@jcallahan-akamai jcallahan-akamai removed the request for review from mjac0bs October 12, 2023 17:58
@mjac0bs mjac0bs self-requested a review October 12, 2023 17:58
@coliu-akamai coliu-akamai removed the request for review from hana-akamai October 12, 2023 18:12
@coliu-akamai
Copy link
Contributor Author

(removing Hana since she's OOO!)

Copy link
Contributor

@abailly-akamai abailly-akamai left a comment

Choose a reason for hiding this comment

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

Looks great! thanks for the cleanup

  • Found no styling regression ✅
  • Found no console errors from new styled components ✅
  • Found no functionality regression on either stackscript or marketplace flows

Left a few comments for some cleanup

@coliu-akamai coliu-akamai added Add'tl Approval Needed Waiting on another approval! and removed Ready for Review labels Oct 16, 2023
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.

Overall, this looked good! Tested Stackscripts landing, create, and details page, rebuilding from Stackscript, and create from Stackscript.

I did notice one minor spacing change between the Revision Note field and the action buttons at the bottom of StackScripts Create -- was this intentional?

Prod This Branch
Screenshot 2023-10-17 at 9 44 42 AM Screenshot 2023-10-17 at 9 44 28 AM

I also noticed two things unrelated to this PR and present in prod, but, I think, worth follow up ticket(s) to address:

  • On the edit form, when a stackscript is submitted without an image, the help icon shifts downward to be off-center once the error appears beneath the text field.
Screen.Recording.2023-10-17.at.9.52.18.AM.mov
  • When a user tries to submit a stackscript without a label and image, receives validation errors, and then comes back into that same edit form, the field values don't repopulate with the label and image data, even though they are present in the API request for that stackscript.

No prepopulated label and image fields:

Screen.Recording.2023-10-17.at.9.53.21.AM.mov

The request that was made when the edit form was loaded, which includes the existing label and image data:
Screenshot 2023-10-17 at 9 54 20 AM

@mjac0bs mjac0bs added Approved Multiple approvals and ready to merge! and removed Add'tl Approval Needed Waiting on another approval! labels Oct 17, 2023
@coliu-akamai
Copy link
Contributor Author

Thanks for catching Mariah! Fixed spacing + created tickets [M3-7280] and [M3-7281] for these bugs!

@coliu-akamai coliu-akamai merged commit ee83522 into linode:develop Oct 18, 2023
11 checks passed
@coliu-akamai coliu-akamai deleted the r-m3-6282-p2 branch November 6, 2023 19:44
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.

4 participants