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

change: [M3-7205] - Dark theme Radio & Toggle improvements #10020

Merged
merged 9 commits into from
Jan 5, 2024

Conversation

abailly-akamai
Copy link
Contributor

@abailly-akamai abailly-akamai commented Dec 22, 2023

Description 📝

This PR improves the styling of the Radio and Toggle components in dark mode (only). It aims to handle default and disabled states styling when the dark mode is turned on.

It does not modify the light theme styling.

Changes 🔄

  • Improve styling for both components (dark mode only)
  • Improve stories for both components
  • Improve tooltip content in Migration panel (unified migration)

Preview 📷

(note: the "Before" screenshot for the Resize modal is for a Linode that is powered off)

Before After
Screenshot 2024-01-02 at 14 37 50 Screenshot 2024-01-02 at 14 33 49
Screenshot 2024-01-02 at 14 37 12 Screenshot 2024-01-02 at 14 35 33

How to test 🧪

Verification steps

  • pull and run code locally
  • run yarn storybook as well
  • Verify Toggle and Radio component stories and the styling with the dark mode
  • Verify styling of Radios and Toggles in CM (ex: user permissions for toggles and resize modal)

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

<Typography fontFamily={theme.font.bold} sx={{ mt: 1 }}>
Your Linode must be powered on to select a warm resize.
</Typography>
)}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It bothered me that the radio group was disabled without an explanation as to why so I added this helper text - also improved the typography for the label ("recommended")

Before After
Screenshot 2024-01-02 at 16 21 57 Screenshot 2024-01-02 at 16 19 35

@abailly-akamai abailly-akamai marked this pull request as ready for review January 2, 2024 21:30
@abailly-akamai abailly-akamai requested a review from a team as a code owner January 2, 2024 21:30
@abailly-akamai abailly-akamai requested review from dwiley-akamai and hana-akamai and removed request for a team January 2, 2024 21:30
Copy link

github-actions bot commented Jan 2, 2024

Coverage Report:
Base Coverage: 79.16%
Current Coverage: 79.16%

@dwiley-akamai dwiley-akamai changed the title change: [M3-705] - Dark theme Radio & Toggle improvements change: [M3-7205] - Dark theme Radio & Toggle improvements Jan 2, 2024
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.

Toggle and Radio component stories and the styling with the dark mode ✅
Radios and Toggles styling in CM ✅

render: () => <Toggle interactive={true} tooltipText={EXAMPLE_TEXT} />,
export const WithTooltip: StoryObj<ToggleProps> = {
render: (args) => (
<Toggle {...args} interactive={true} tooltipText={EXAMPLE_TEXT} />
Copy link
Contributor

Choose a reason for hiding this comment

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

In Storybook on this branch, it doesn't seem like toggling interactive actually makes a difference in the behavior now: once the tooltip pops up, you're always able to hover over it without it disappearing. In prod Storybook, "Tooltip Toggle" has a non-interactive tooltip (it disappears when you try to hover over it) and "Interactable Tooltip Toggle" has an interactive tooltip.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch! unfortunately that behavior can't be toggled in storybook easily cause it would have to be remounted for the prop to take effect. That being said I added the tooltip on the first story so that the behavior remains consistent with the previous rendition of this component in Storybook

@hana-akamai
Copy link
Contributor

Dark mode changes LGTM.

I did notice a weird bug with the resize modal though (seems to also exist in prod). If you click out of the resize modal, the Auto Resize Disk box gets unchecked and the Radio check moves to Warm resize (if Cold was checked)

Screen.Recording.2024-01-03.at.5.02.14.PM.mov

(IP addresses redacted)

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.

Changes look good to me

@abailly-akamai
Copy link
Contributor Author

abailly-akamai commented Jan 4, 2024

@hana-linode the state management in this component is pretty complex and brittle so since it is a small visual bug that does not affect the user experience I will leave it like this for now but created a ticket to look at it further: M3-7590

@mjac0bs mjac0bs added the Add'tl Approval Needed Waiting on another approval! label Jan 4, 2024
@hana-akamai hana-akamai added Approved Multiple approvals and ready to merge! and removed Add'tl Approval Needed Waiting on another approval! labels Jan 4, 2024
@abailly-akamai
Copy link
Contributor Author

Confirmed smoke-community-stackscrips.spec.ts failure was flaky

@abailly-akamai abailly-akamai merged commit 3a1085b into linode:develop Jan 5, 2024
17 of 18 checks passed
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