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-6273] - MUI v5 Migration - SRC > Features > Lish #9774

Merged
merged 11 commits into from
Oct 11, 2023

Conversation

jaalah-akamai
Copy link
Contributor

@jaalah-akamai jaalah-akamai commented Oct 10, 2023

Description 📝

Refactored Lish as part of MUI v5 migration

Changes 🔄

  • CircleProgress.tsx - Allow sx to be passed in as props
  • ReachTab.tsx - Remove !important that was causing styling issues with Lish tabs
  • Lish.tsx - No need to import css files in useEffect, we can import normally.
    • Vertically center the CircleProgress when loading
    • Remove use of Fragment and return null instead
    • Use Styled api instead of makeStyles

Preview 📷

Before After
Screenshot 2023-10-10 at 10 21 12 AM Screenshot 2023-10-10 at 10 23 14 AM
Screenshot 2023-10-10 at 10 27 34 AM Screenshot 2023-10-10 at 10 29 30 AM
Before After
lish-cloud-dark.mp4
lish-local-dark.mp4
lish-cloud-light.mp4
lish-local-light.mp4

How to test 🧪

Prerequisites

  • Must have a Linode

Reproduction steps

  • Go to linodes detail page
  • Open "Launch LISH Console"

Verification steps

  • Observe hover states for tabs are fixed
  • Observe css styles are still working
  • Observe loading state is vertically centered (alternatively if you're pulling this down locally, you can just set the isLoading condition to true to test

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

import Glish from './Glish';
import Weblish from './Weblish';

const AUTH_POLLING_INTERVAL = 2000;

const useStyles = makeStyles((theme: Theme) => ({
notFound: {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was never used

},
color: '#f4f4f4 !important',
},
progress: {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This has no effect whatsoever, so I removed since we're centering this anyways

@@ -11,7 +11,7 @@ const useStyles = makeStyles()((theme: Theme) => ({
},
'&:hover': {
backgroundColor: theme.color.grey7,
color: `${theme.palette.primary.main} !important`,
Copy link
Contributor Author

@jaalah-akamai jaalah-akamai Oct 10, 2023

Choose a reason for hiding this comment

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

This !important was added because there was no hover style for selected tabs. I added that styled and the hover color for selected tabs should stay blue.

You can test this by going through the create linode flow and looking at the plan tabs
Screenshot 2023-10-10 at 11 41 08 AM

@jaalah-akamai jaalah-akamai marked this pull request as ready for review October 10, 2023 15:44
@jaalah-akamai jaalah-akamai requested a review from a team as a code owner October 10, 2023 15:44
@jaalah-akamai jaalah-akamai requested review from bnussman-akamai and abailly-akamai and removed request for a team October 10, 2023 15:44
packages/manager/src/components/ReachTab.tsx Outdated Show resolved Hide resolved
packages/manager/src/components/ReachTab.tsx Outdated Show resolved Hide resolved
jaalah-akamai and others added 2 commits October 10, 2023 16:58
Co-authored-by: Banks Nussman <115251059+bnussman-akamai@users.noreply.github.com>
Co-authored-by: Banks Nussman <115251059+bnussman-akamai@users.noreply.github.com>
@tyler-akamai
Copy link
Contributor

tyler-akamai commented Oct 10, 2023

Reproduced original state ✅

Ensured that:

  • Hover states for tabs are fixed
  • CSS styles are still working
  • Loading state is vertically centered

@tyler-akamai
Copy link
Contributor

tyler-akamai commented Oct 10, 2023

Should we left align the terminal text for Glish like we have it for Weblish?

Weblish Glish
Screenshot 2023-10-10 at 6 38 09 PM Screenshot 2023-10-10 at 6 39 40 PM

After doing some digging, it seems like the issue isn't the alignment but rather the margins of the <Canvas /> within the <VncScreen /> component in src/features/Lish/Glish.tsx:

Screenshot 2023-10-10 at 7 14 49 PM

@tyler-akamai
Copy link
Contributor

tyler-akamai commented Oct 10, 2023

Also, when you say: "Loading state is vertically centered",

do you mean the loading circle progress icon is horizontally centered? I wouldn't consider it vertically centered (Since its closer to the top), but I do like how it looks now.

I just wanted to confirm before checking it off.

@tyler-akamai
Copy link
Contributor

Good work so far!

@tyler-akamai tyler-akamai self-requested a review October 10, 2023 23:17
@jaalah-akamai
Copy link
Contributor Author

do you mean the loading circle progress icon is horizontally centered?

  • I was mainly referring to the main spinner that shows upon opening the console, not the ones for each tab. However, I just centered those too.
  • Fixed the margin issue

@tyler-akamai
Copy link
Contributor

tyler-akamai commented Oct 11, 2023

Still seems like there is too much spacing on the left hand side.

Not sure if this is a problem that should be addressed in this PR or another. Or maybe its not a problem at all, thoughts?

Screen.Recording.2023-10-11.at.9.47.06.AM.mov

@jaalah-akamai
Copy link
Contributor Author

I think I misunderstood your concern - lets sync offline

@tyler-akamai
Copy link
Contributor

Confirmed everything works as expected and the text is left aligned ✅

Screenshot 2023-10-11 at 10 04 06 AM

Screenshot 2023-10-11 at 10 03 53 AM

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