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: [DI-24111] - Updated hovering color for dashboard icons #11883

Merged
merged 8 commits into from
Mar 20, 2025

Conversation

nikhagra-akamai
Copy link
Contributor

@nikhagra-akamai nikhagra-akamai commented Mar 19, 2025

Description 📝

Added blue color to icon on hover in dashboards

Changes 🔄

List any change(s) relevant to the reviewer.

  1. Updated global filters file to add blue color on hover
  2. Updated zoomer.tsx file to add blue color on hover

Target release date 🗓️

Please specify a release date (and environment, if applicable) to guarantee timely review of this PR. If exact date is not known, please approximate and update it as needed.

Preview 📷

Include a screenshot or screen recording of the change.

🔒 Use the Mask Sensitive Data setting for security.

💡 Use <video src="" /> tag when including recordings in table.

Before After
Screenshot 2025-03-19 at 11 32 01 AM Screenshot 2025-03-19 at 12 20 18 PM
Screenshot 2025-03-19 at 11 32 09 AM Screenshot 2025-03-19 at 12 20 33 PM
Screenshot 2025-03-19 at 11 32 17 AM Screenshot 2025-03-19 at 12 20 24 PM

How to test 🧪

  1. Switch as mock user
  2. Go to metrics from mega menu
  3. select dashboard & required filters
  4. hover on refresh and maximize/minimize button to check its changing color
Author Checklists

As an Author, to speed up the review process, I considered 🤔

👀 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


  • I have read and considered all applicable items listed above.

As an Author, before moving this PR from Draft to Open, I confirmed ✅

  • All unit tests are passing
  • TypeScript compilation succeeded without errors
  • Code passes all linting rules

@nikhagra-akamai nikhagra-akamai marked this pull request as ready for review March 19, 2025 09:43
@nikhagra-akamai nikhagra-akamai requested a review from a team as a code owner March 19, 2025 09:43
@nikhagra-akamai nikhagra-akamai requested review from bnussman-akamai and coliu-akamai and removed request for a team March 19, 2025 09:43
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.

Our <IconButton />'s default styles are to turn blue on hover. We shouldn't need to apply custom styles to accomplish this.

Also, the Icons are completely broken in dark mode

Screen.Recording.2025-03-19.at.11.42.05.AM.mov

I recommend the following

  • Open up zoomin.svg, zoomout.svg, refresh.svg. Change all fills and strokes to use currentColor rather than their hardcoded colors
  • Remove styled icons and one-off styles completely

These changes will make the code much cleaner, simple, and has the added benefits of pulling styles from our theme. The more one-off styles we have, the harder it will be to make a cohesive application with a good user experience.

@nikhagra-akamai
Copy link
Contributor Author

thanks of code cleanup suggestions @bnussman-akamai I've modified the changes accordingly. I've added color prop in icons button as default button color is light but as per mockups it is a bit darker.

@linode-gh-bot
Copy link
Collaborator

Cloud Manager UI test results

🎉 539 passing tests on test run #7 ↗︎

❌ Failing✅ Passing↪️ Skipped🕐 Duration
0 Failing539 Passing3 Skipped104m 6s

@nikhagra-akamai nikhagra-akamai merged commit 6d7a586 into linode:develop Mar 20, 2025
25 checks passed
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.

5 participants