Skip to content

refactor: [M3-6963] - MUI v5 Migration - Src > Component > Breadcrumb#9877

Merged
tyler-akamai merged 4 commits intolinode:developfrom
tyler-akamai:M3-6963-css-migration-finalcrumbprefix
Nov 9, 2023
Merged

refactor: [M3-6963] - MUI v5 Migration - Src > Component > Breadcrumb#9877
tyler-akamai merged 4 commits intolinode:developfrom
tyler-akamai:M3-6963-css-migration-finalcrumbprefix

Conversation

@tyler-akamai
Copy link
Contributor

@tyler-akamai tyler-akamai commented Nov 6, 2023

Description 📝

MUI v5 Migration - Src > Component > Breadcrumb

Changes 🔄

List any change relevant to the reviewer.

  • MakeStyle --> Styled API + sx props

Preview 📷

Include a screenshot or screen recording of the change

💡 Use <video src="" /> tag when including recordings in table.

Before After
Screenshot 2023-11-06 at 3 58 25 PM Screenshot 2023-11-06 at 3 58 09 PM
Screenshot 2023-11-06 at 3 59 50 PM Screenshot 2023-11-06 at 3 59 26 PM
Screenshot 2023-11-06 at 4 00 33 PM Screenshot 2023-11-06 at 4 01 00 PM

How to test 🧪

Prerequisites

(How to setup test environment)

  • There are no prerequisites

Reproduction steps

(How to reproduce the issue, if applicable)

  • There are no reproduction steps since this is a refactor

Verification steps

(How to verify changes)

  • Verify that the Breadcrumbs match the page for each product (Linode, NodeBalancers, Firewalls, etc.)
  • Verify that the Breadcrumb matches the instance of the details page (Click on a specific Linode, NodeBalancer, Firewall, etc.)
  • Verify that you can edit the name of the instance (ex. you can edit the name of the Linode that you selected via the breadcrumb)
  • Verify that the name is saved and used in the landing page

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

@tyler-akamai tyler-akamai changed the title mui v5 migration for breadcrumbs refactor: [M3-6963] - MUI v5 migration - Src > Component > Breadcrumb Nov 6, 2023
@tyler-akamai tyler-akamai changed the title refactor: [M3-6963] - MUI v5 migration - Src > Component > Breadcrumb refactor: [M3-6963] - MUI v5 Migration - Src > Component > Breadcrumb Nov 6, 2023
},
className
)}
<StyledRootDiv
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The original code has two makeStyle classes. One had the bulk of the styling, the other just has a single style element that would be added if hasError == true. So instead of making two separate Styled api components, I created one with the basic styling, and the have an sx component that adds margin to the bottom if hasError == true

@tyler-akamai tyler-akamai changed the title refactor: [M3-6963] - MUI v5 Migration - Src > Component > Breadcrumb refactor: [M3-6963] - MUI v5 Migration - Src > Component > Breadcrumb Nov 6, 2023
@tyler-akamai tyler-akamai marked this pull request as ready for review November 6, 2023 21:02
@tyler-akamai tyler-akamai requested a review from a team as a code owner November 6, 2023 21:02
@tyler-akamai tyler-akamai requested review from cliu-akamai and cpathipa and removed request for a team November 6, 2023 21:02
Copy link
Member

@bnussman-akamai bnussman-akamai left a comment

Choose a reason for hiding this comment

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

Didn't notice any regressions 🔎

Copy link
Contributor

@cpathipa cpathipa left a comment

Choose a reason for hiding this comment

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

LGTM! Verified Breadcrumbs, no regressions found.

@mjac0bs mjac0bs added the Approved Multiple approvals and ready to merge! label Nov 9, 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.

Thanks @tyler-akamai! One nitpick: there are some linter warnings in the files for imports that would be nice to fix. I also left an optional suggestion about the bottom margin for the error on label.

Confirmed the following:
✅ Verify that the Breadcrumbs match the page for each product (Linode, NodeBalancers, Firewalls, etc.)
✅ Verify that the Breadcrumb matches the instance of the details page (Click on a specific Linode, NodeBalancer, Firewall, etc.)
✅ Verify that you can edit the name of the instance (ex. you can edit the name of the Linode that you selected via the breadcrumb)
✅ Verify that the name is saved and used in the landing page

@tyler-akamai tyler-akamai merged commit 26b544c into linode:develop Nov 9, 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.

5 participants