-
Notifications
You must be signed in to change notification settings - Fork 40
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
Remove the hover text-decoration transition in InlineLink
, Accordion
, Timeline
and align Prose
with the updated InlineLink
styles
#913
base: main
Are you sure you want to change the base?
Conversation
…er, but since this is 1px transition, transitioning it is practically useless
🦋 Changeset detectedLatest commit: fa028d2 The changes in this PR will be included in the next version bump. This PR includes changesets to release 6 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
🟢 No design token changes found |
|
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.
LGTM, thanks @stamat!
Hold off for Rez's ✅ before merging though please
Thanks for tidying up the InlineLink component @stamat ✨ 🧹 Before we merge this, a few things to add...
|
@rezrah yo, I don't know what does this mean: |
@rezrah ah I see I see, the design token for the variable right? |
… it with the rest
InlineLink
InlineLink
, Accordion
, Timeline
and align Prose
with the updated InlineLink
styles
"@primer/react-brand": patch | ||
--- | ||
|
||
Remove the hover text-decoration transition in `InlineLink`, `Accordion`, `Timeline` and align `Prose` with the updated `InlineLink` styles |
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.
Great, thanks @stamat.
On the off-chance that someone was relying on that downstream, let's provide a migration path:
- var(--brand-InlineLink-transition-timing)
+ var(--brand-animation-duration-fast)
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.
Remove the hover text-decoration transition in `InlineLink`, `Accordion`, `Timeline` and align `Prose` with the updated `InlineLink` styles | |
Removed a redundant hover text-decoration transition in `InlineLink`, `Accordion`, `Timeline` and align `Prose`. | |
Also removed the following css variable: `--brand-InlineLink-transition-timing`. If you were using this variable, please use `--brand-animation-duration-fast` instead. |
@@ -252,10 +252,8 @@ details[open] > .Accordion__summary.Accordion__summary--default .Accordion__summ | |||
color: var(--brand-InlineLink-color-rest); | |||
position: relative; | |||
text-decoration: underline; | |||
text-decoration-color: var(--brand-InlineLink-color-rest); |
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.
@stamat i think removing this has caused a visual regression. As you're trying to remove the transition, i'm guessing it's unintentional. Could we restore it please?
…ed to be declared" This reverts commit 75990f7.
Summary
We can't animate the text-decoration-thickness, we can animate a border, but since this is 1px transition, transitioning it is practically useless. Tested it on dotcom.
List of notable changes:
transition: var(--brand-InlineLink-transition-timing) text-decoration;
fromInlineLink
styles.transition: var(--brand-InlineLink-transition-timing) text-decoration;
fromAccordion
styles.transition: var(--brand-InlineLink-transition-timing) text-decoration;
fromTimeline
styles.Prose
inline link styles with the rest, since the animation didn't work anyways.What should reviewers focus on?
InlineLink
Steps to test:
Supporting resources (related issues, external links, etc):
Contributor checklist:
update snapshots
label to the PR)Reviewer checklist:
Screenshots: