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-6304] - MUI v5 Migration - ConditionalWrapper #9002

Conversation

bnussman-akamai
Copy link
Member

Description 📝

  • Deletes <ConditionalWrapper /> because it was a bit extra and only consumed once
  • Cleans up components that consume it
  • Fixes some styles that were broken when we migrated from MUIv4 to MUIv5

How to test 🧪

  • The main component this affects is Docs links
    • Verify that docs links look okay

@@ -12,7 +10,7 @@ const useStyles = makeStyles()((theme: Theme) => ({
display: 'flex',
alignItems: 'flex-start',
cursor: 'pointer',
padding: theme.spacing(1) + theme.spacing(0.5),
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 was probably left over from MUI v4. theme.spacing() use to return a number, now it returns a string so this line actually caused string concatenation which is not what we want. This might result in some minor UI changes, but this restores the original intention.

@@ -48,7 +46,7 @@ const useStyles = makeStyles()((theme: Theme) => ({
},
},
left: {
left: -(theme.spacing(1) + theme.spacing(0.5)),
Copy link
Member Author

Choose a reason for hiding this comment

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

Same as other comment. string concatenation here was probably not intended

@cypress
Copy link

cypress bot commented Apr 14, 2023

Passing run #2984 ↗︎

0 149 3 0 Flakiness 0

Details:

remove ConditionalWrapper and fix components that consumed it
Project: Cloud Manager E2E Commit: 8d3939d0fa
Status: Passed Duration: 14:46 💡
Started: Apr 14, 2023 7:03 PM Ended: Apr 14, 2023 7:17 PM

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings.

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.

The Docs button appears to have drifted a few pixels to the left, but this LGTM otherwise

@bnussman-akamai bnussman-akamai merged commit a624f83 into linode:develop Apr 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants