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-6343] - MUI v5 Migration - Grid v2 for Features #8985

Merged
merged 21 commits into from
Apr 14, 2023

Conversation

jaalah-akamai
Copy link
Contributor

@jaalah-akamai jaalah-akamai commented Apr 11, 2023

Description 📝

Migrating to MUI Grid v2. This covers everything in the src/features directory (excluding Longview which is forthcoming) and any additional files that needed to be cleaned up.

Note: In several places, you MAY notice slight padding variations due to overly excessive Grid usage. In places like this, I made my best judgment and consulted with UX/Design. You'll notice in several places the use of padding: 0 on Grid elements. This was to match the existing design as much as possible until we get a chance to revisit these areas. Feel free to highlight them, and I'll comment!

Major Changes 🔄

  • MainContent.tsx - You'll notice slightly more padding for the main content container. This is because the page is actually respecting the original padding of the container now.
  • Changed Grid import from src/components/Grid to @mui/material/Unstable_Grid2.
    • The item attribute is no longer needed for Grid.
    • Most Grid elements with the container attribute needed a spacing attribute as part of the change
  • updateFor has been removed off the Grid component. There's no need to micromanage how/when a component should re-render like this anymore.

Refactored

  • LinodeEntityDetail.tsx - Very large file, did some major refactoring here to get things looking like they did before. Check all breakpoints. You'll notice that DNS Resolvers has been updated to use CSS Grid and its layout on smaller breakpoints is slightly different. UX approves 👍
  • DNSResolvers.tsx - Simplified code and removed makeStyles
  • NodeBalancerDetail.tsx - Using LandingHeader and removed makeStyles
  • RenderNotification.tsx - No need for Grid, this was an easy refactor
  • useFormattedNotifications.tsx - Due to changes in RenderNotification this needed a slight refactor
  • LinodeAdvancedConfigurationsPanel.tsx - Didn't seem like the styles here were being used
  • AlertSection.tsx - Instead of forcing changes to work, I saw refactoring was more simple.

Replaced Grid w/ Box component

There was no need for a Grid components in these files, so I swapped them with Box or removed the Grid completely.

  • ExpandableTicketPanel.tsx - Removed lots of extra Grid components in this file
  • AutoEnroll.tsx
  • DatabaseSummaryConnectionDetails.tsx
  • FirewallRuleTable.tsx
  • SupportSearchLanding.tsx
  • ImageSelect.tsx
  • KubeConfigDisplay.tsx
  • NodeBalancerConfigurations.tsx
  • TablesPanel.tsx - This is under NodeBalancers
  • VolumeTableRow.tsx

Deleted

  • features/Domains/DomainsTableWrapper.tsx - This didn't seem to be used

How to test

  1. These changes touch the core features we offer, so a page by page scan is required
  2. Go to https://cloud.linode.com/ and compare side by side with local
  3. There are slight changes as noted above and you may notice slight padding changes. Anything agregious, just flag in this PR and I'll take a look at.
  4. Start with larger breakpoints and work your way down.
Where to view changes? Open me! 👇
Component Where to view or Desc? Screenshot
AutoEnroll
http://localhost:3000/account/settings
Click on "enable now" link to open drawer. If you're a "managed" user, you may not be able to see this.
DatabaseSummaryConnectionDetails
http://localhost:3000/databases/postgresql/{ID}/summary
Screen Shot 2023-04-12 at 10 18 49 AM
Firewalls Rules
http://localhost:3000/firewalls/{ID}
Screen Shot 2023-04-12 at 10 21 19 AM
ImageSelect
http://localhost:3000/stackscripts/create
Screen Shot 2023-04-12 at 10 28 02 AM
KubeConfigDisplay
http://localhost:3000/kubernetes/clusters/{ID}/summary
Screen Shot 2023-04-12 at 10 30 27 AM
NodeBalancerConfigurations
http://localhost:3000/nodebalancers/{ID}/configurations
Configurations tab, lots of refactoring here Screen Shot 2023-04-12 at 10 33 39 AM
Linode Summary View
http://localhost:3000/linodes?view=grid
Tags were using a 1400 breakpoint which is not a supported breakpoint. These will now change layout at < 1280 Screen Shot 2023-04-12 at 9 20 59 AM
Linode Detail
http://localhost:3000/linodes/{ID}/networking
DNS Resolvers has been refactored Screen Shot 2023-04-12 at 9 35 58 AM
Notifications
http://localhost:3000/profile/settings
Use mock service worker > open events dropdown Screen Shot 2023-04-12 at 9 57 00 AM
ExpandableTicketPanel
http://localhost:3000/support/tickets
Use MSW Screen Shot 2023-04-12 at 10 02 05 AM
Entity Transfer Create
http://localhost:3000/account/service-transfers/create
http://localhost:3000/account/service-transfers
Firewalls Linodes
http://localhost:3000/firewalls/{ID}/linodes
Screen Shot 2023-04-12 at 10 09 24 AM
Search Results
http://localhost:3000/support/search/?query=test
Screen Shot 2023-04-12 at 10 11 00 AM
User Menu Top navigation user menu Screen Shot 2023-04-12 at 11 29 52 AM

QA Checklist

Tag your name next to the ones you've QA'd

  • Linodes
  • Volumes
  • NodeBalancers
  • Firewalls
  • Stackscripts
  • Images
  • Domains
  • Databases
  • Kubernetes
  • Object Storage
  • Marketplace - Shouldn't be any changes here
  • Account
  • Help & Support

},
},
}));

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Almost all these styles were un-used. 🗑️

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I could've gone further with this file, but I left it intact for now

>
<Grid item className={classes.addNewWrapper}>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

No need for all the spacing adjustment from addNewWrapper anymore.

@jaalah-akamai jaalah-akamai marked this pull request as ready for review April 12, 2023 14:36
@jaalah-akamai jaalah-akamai marked this pull request as draft April 12, 2023 14:41
@jaalah-akamai jaalah-akamai marked this pull request as ready for review April 12, 2023 15:33
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.

Things generally look good so far, can we decrease the spacing here across all entities, it has gotten larger in this PR

Screenshot 2023-04-13 at 10 26 12 AM

@jaalah-akamai
Copy link
Contributor Author

Things generally look good so far, can we decrease the spacing here across all entities, it has gotten larger in this PR

Screenshot 2023-04-13 at 10 26 12 AM

Yea I was going back and forth with that, but yea let's do it!

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.

I still have a few more places in the app to check and a lot of code to sift through, but it's looking good so far.

One small thing I noticed is that the graphics on the Referrals page are closer together than they were previously, do we want to keep that?

Copy link
Contributor

@hana-akamai hana-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 TagsPanel is no longer flush to the right side

image

Mobile spacing for the firewalls rule table seems a bit tight now
This branch
image

Prod
image

Mobile spacing for Kube display is a bit off/tight
image

"Port Configuration" display disappeared

The extra white padding around the accordion header looks less clean imo
This branch
image

Prod
image

@bnussman-akamai
Copy link
Member

@hana-linode The removal of Port Configuration was done by me in #8964, it was intentional

@cypress
Copy link

cypress bot commented Apr 13, 2023

1 flaky tests on run #2988 ↗︎

0 149 3 0 Flakiness 1

Details:

Update changelog
Project: Cloud Manager E2E Commit: 4de6b343d4
Status: Passed Duration: 14:47 💡
Started: Apr 14, 2023 8:39 PM Ended: Apr 14, 2023 8:54 PM
Flakiness  cypress/e2e/objectStorage/object-storage.e2e.spec.ts • 1 flaky test

View Output Video

Test Artifacts
object storage end-to-end tests > can upload, access, and delete objects Output Screenshots Video

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

@jaalah-akamai
Copy link
Contributor Author

Thanks @hana-linode let me know if things are looking better now. I actually improved the alignment of the inbound rules dropdown to always match the action column since I saw that was the intent cc: @mjac0bs

fix.mp4

Screen Shot 2023-04-13 at 4 36 33 PM

Screen Shot 2023-04-13 at 4 38 34 PM

@mjac0bs
Copy link
Contributor

mjac0bs commented Apr 13, 2023

I actually improved the alignment of the inbound rules dropdown to always match the action column since I saw that was the intent

🥇 @jaalah-akamai Thank you for doing that. I remember it being just slightly off in the accessibility fix and it haunted me a little. Giving the Firewalls page and the rest of this PR a closer look now!

@hana-akamai
Copy link
Contributor

Ah I probably should have cropped the image more, but I meant that the mobile spacing between table columns looks a bit tight now

This branch
image

Prod
image

@jaalah-akamai
Copy link
Contributor Author

jaalah-akamai commented Apr 13, 2023

@hana-linode gotcha! should be good now, increased spacing slightly for the first item from 30% to 32% because we couldn't use margins (had to use padding) to create space like we did in prod. I didn't want any words breaking to new lines as things got smaller.

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.

I'm still working my way through pages at each breakpoint, but here are a few spacing differences so far:

Firewalls -- This looks better now! I'm happy with the changes here.
As @hana-linode pointed out, spacing between columns is tight as we move to smaller screens. This is especially noticeable in columns if a user has a long rule name or list of many IPs.

This branch Prod
Screenshot 2023-04-13 at 2 00 18 PM Screenshot 2023-04-13 at 2 00 04 PM

Linode Landing -- The collapsed tags "..." and the "Add a Tag" button lack some spacing between so they meld into one.

This branch Prod
Screenshot 2023-04-13 at 2 11 05 PM Screenshot 2023-04-13 at 2 11 11 PM

StackScripts

  • Thank you for fixing Image Select > Target Images! I recently made a ticket for this because I noticed our wandering TooltipIcons.

Linodes Details > Storage > Add a Disk - in the same ticket I made for the above, I noticed another spot with the misaligned icon. On this branch, it's still looking like it needs some adjustment on the Image field.

This branch Prod
Screenshot 2023-04-13 at 2 50 53 PM Screenshot 2023-04-13 at 2 51 21 PM

Images -- Looks good to me, other than: just noticed this minor styling bug while testing (also in prod) that might be out of scope of this PR, but it would be cool if we could get our TooltipIcon on the Region field in Upload Image to stop having this weird spacing at the 599 breakpoint.

Screen.Recording.2023-04-13.at.2.32.16.PM.mov

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.

LGTM 🚀

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.

Help and Support:

At a width of 599px on down, there is some misalignment happening that I think we could fix here (also currently in prod):
image

Profile > Login & Authentication: (https://localhost:3000/profile/auth)
TPA provider options are having some issues now at smaller breakpoints.

Prod:

Screen.Recording.2023-04-14.at.10.02.03.AM.mov

This branch:

Screen.Recording.2023-04-14.at.10.01.45.AM.mov

Linode Create:

This branch Prod
Screenshot 2023-04-14 at 10 35 04 AM Screenshot 2023-04-14 at 10 34 49 AM

Kubernetes:
I know we recently did some work on this page... the margin on the right seems to small to me and feels better in prod, but I didn't review the other ticket when this was refactored.

This branch Prod
Screenshot 2023-04-14 at 10 29 30 AM Screenshot 2023-04-14 at 10 29 37 AM

|

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.

I think this is in a good enough state to proceed! I think any outstanding issues could be resolved in this PR or after this is merged

Copy link
Contributor

@hana-akamai hana-akamai left a comment

Choose a reason for hiding this comment

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

Other than Mariah's comments, this is looking good to me 🚀

@jaalah-akamai
Copy link
Contributor Author

Here are the updates - thanks everybody!

01.mp4
02.mp4
fix01.mp4
fix02.mp4

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 for the fixes! 🚀 Can we add this to the Tech Stories section in the changelog?

@jaalah-akamai jaalah-akamai merged commit 8be3dc4 into linode:develop Apr 14, 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.

6 participants