Skip to content

refactor: [M3-6274] - MUI v5 Migration - SRC > Features > Longview Pt1: Longview Detail#9600

Merged
coliu-akamai merged 18 commits intolinode:developfrom
coliu-akamai:r-m3-6274
Sep 12, 2023
Merged

refactor: [M3-6274] - MUI v5 Migration - SRC > Features > Longview Pt1: Longview Detail#9600
coliu-akamai merged 18 commits intolinode:developfrom
coliu-akamai:r-m3-6274

Conversation

@coliu-akamai
Copy link
Contributor

@coliu-akamai coliu-akamai commented Aug 28, 2023

Description 📝

  • migrating styling for src > features >longview > longview detail. Splitting this ticket up into two PRs (this one is still 50 changed files though yikes 😅 ) -- this one, which covers LongviewDetail, and a second one, which will cover LongviewLanding -- so that it's easier to test / catch regressions

Major Changes 🔄

Inside LongivewDetail package,

  • remove React.FC
  • change to named exports (unless components were being lazily exported, then kept as is)
  • remove makeStyles
  • some files had a lot of common styling, so moved all this to CommonStyles.styles.ts (similar to this ticket from way back)

How to test 🧪

  • navigate to Longview Details and ensure that everything looks as expected, and that all functionality is still the same

@coliu-akamai coliu-akamai self-assigned this Aug 28, 2023
@coliu-akamai coliu-akamai changed the title refactor: [M3-6274] - MUI v5 Migration - SRC > Features > Longview Pt1: Longview Detail refactor: [M3-6274] - MUI v5 Migration - SRC > Features > Longview Pt1: Longview Detail Aug 28, 2023
@coliu-akamai coliu-akamai marked this pull request as ready for review August 29, 2023 15:06
@tyler-akamai
Copy link
Contributor

tyler-akamai commented Aug 31, 2023

So it seems like there was a styling regression at some point with the longview page:

if you run:
git checkout 6a5d547

you can see the original styling. For some reason though, I am getting a 'clamp' error so I can only see it for a split second.

I am also getting this error in prod as well, not sure what is causing it.

I'm not sure if this would be part of this ticket or another.

@coliu-akamai
Copy link
Contributor Author

coliu-akamai commented Aug 31, 2023

@tyler-akamai I think it's the commit right after that one that changes things on Longview (but it's weird since it doesn't touch any longview files...) just to confirm I'm not going crazy, can you run git checkout 79f16f89f6086814431dd59012eeb0855d67c802?

edit: tried some commits before 6a5d547, and the longview landing page looks like what prod currently is, so I don't think it was 79f15f8

@coliu-akamai
Copy link
Contributor Author

coliu-akamai commented Aug 31, 2023

Commenting here for visibility: I found (at least one of) the issue(s), related to this pr: #9488 -- basically, changing from default to named exports (and removing the /core folder) caused us to change the file we were importing from, which then changed styling.

Using the LongviewClients.tsx file as an example:

  • it it used to import a Grid using a default export (import Grid from 'src/components/Grid';) which pointed to the src/components/Grid/Grid.tsx file.
  • now the import line is import { Grid } from 'src/components/Grid' which points to src/components/Grid.tsx (this file used to be in a core folder, so previously, if we wanted this file we would have needed to import from src/components/core/Grid)
  • the grid from src/components/Grid/Grid.tsx had some additional styling on it (namely spacing={2}) depending on the props passed in, so that's causing a styling difference for the LongviewClients.tsx file. This may also be the problem for the other files in src/features/Longview, haven't really poked around yet
  • to see the differences, try git checkout e556e59 (the commit that was pr refactor: Finish organizing components #9488) vs git checkout 84f1083 (the commit right before the pr) and navigate to the Longview landing page

++ will be handling in this ticket and #9612

@coliu-akamai
Copy link
Contributor Author

nvm, just put up a super quick fix here: #9619 + after that gets merged in, will update this and the p2 pr

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

Screenshot 2023-09-11 at 2 21 04 PM

these changes should fix the gap issue

@coliu-akamai
Copy link
Contributor Author

Thanks Jaalah! I'd accidentally thought the Listening services/Active connection were supposed to look like that and that it was an interesting UI decision oops, should have double checked 😅

coliu-akamai and others added 3 commits September 11, 2023 15:03
…bs/LongviewDetailOverview.tsx

Co-authored-by: Jaalah Ramos <125309814+jaalah-akamai@users.noreply.github.com>
…bs/ListeningServices/ListeningServices.tsx

Co-authored-by: Jaalah Ramos <125309814+jaalah-akamai@users.noreply.github.com>
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.

🚀 Thanks for great work on this!

@bnussman-akamai bnussman-akamai added Approved Multiple approvals and ready to merge! and removed Add'tl Approval Needed Waiting on another approval! labels Sep 12, 2023
@coliu-akamai coliu-akamai merged commit dfeb7b8 into linode:develop Sep 12, 2023
@coliu-akamai coliu-akamai deleted the r-m3-6274 branch October 6, 2023 20:14
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