-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Fixed notice hover state to be compatible with removed IsReskinned flag #99542
Conversation
Jetpack Cloud live (direct link)
Automattic for Agencies live (direct link)
|
This PR modifies the release build for the following Calypso Apps: For info about this notification, see here: PCYsg-OT6-p2
To test WordPress.com changes, run |
This PR does not affect the size of JS and CSS bundles shipped to the user's browser. Generated by performance advisor bot at iscalypsofastyet.com. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I must say I don't like the looks of the notice. It's difficult to distinguish it from the background, and the inactive "Next steps" link doesn't have a lot of contrast. Do we plan to refactor that component completely anytime soon? cc @nuriapenya
@@ -282,7 +282,7 @@ a.notice__action { | |||
} | |||
|
|||
&:hover { | |||
color: var(--color-text-inverted); | |||
color: var(--studio-gray-80); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use a color token instead of a color code? It would be --color-accent-80
in this case, but it would be even better if we could use an explicit --hover
token.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see that we had another layout in mind before the last week's changes: p9Jlb4-fmP-p2
cc @nuriapenya
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I am trying to track down at what point we changed the Calypso notice to look "lighter". I agree with @zaguiini , it's currently not visible. Started a thread here in Slack: p1739002922765779-slack-C9EJ7KSGH
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This color change should live under :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see that discussion in Slack, but since this is currently broken in production I feel like I should merge this quick fix.
Follow-up of #99112
Related to the Focused Launchpad: pet6gk-1T7-p2
Proposed Changes
Before:
Screen.Recording.2025-02-10.at.09.22.10.mov
After:
Screen.Recording.2025-02-10.at.09.30.46.mov
Why are these changes being made?
Testing Instructions
flags=home/launchpad-first
Or follow the video:
Screen.Recording.2025-02-10.at.10.46.43.mov
Pre-merge Checklist