-
Notifications
You must be signed in to change notification settings - Fork 60
Remove the hover text-decoration transition in InlineLink, Accordion, Timeline and align Prose with the updated InlineLink styles
#913
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
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 |
|
joshfarrant
left a comment
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
InlineLinkInlineLink, 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. |
| 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;fromInlineLinkstyles.transition: var(--brand-InlineLink-transition-timing) text-decoration;fromAccordionstyles.transition: var(--brand-InlineLink-transition-timing) text-decoration;fromTimelinestyles.Proseinline link styles with the rest, since the animation didn't work anyways.What should reviewers focus on?
InlineLinkSteps to test:
Supporting resources (related issues, external links, etc):
Contributor checklist:
update snapshotslabel to the PR)Reviewer checklist:
Screenshots: