-
Notifications
You must be signed in to change notification settings - Fork 37
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
Add 'torchlight' effect to cards in dark mode #468
Conversation
🦋 Changeset detectedLatest commit: 13a2e0c 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 |
🟢 No visual differences foundOur visual comparison tests did not find any differences in the UI. |
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.
Preview looks great 🚀
Nothing blocking, mostly comments/questions:
- Theming with accents makes a lot of sense, can we also have a neutral color in case of no specific theme?
- Can you confirm that we will allow to not active this effect as it may not be relevant everywhere it is used?
- Can we trigger the CTA/link hover effect when the card is hovered?
Thanks for the quick review @nsolerieu ⚡
The default css variable value for accent color is pink, and is a global setting. It can be customised to be more neutral if needed by overriding the token.
Right now, you can't turn it off unless you have 'reduced motion' setting enabled at OS level. We previously spoke about this replacing the current scale up effect entirely. After you expecting that to continue being the fallback if we disable the skew+light effect?
👍 unrelated bug, but yeah.. i'll try to fix that before we merge this or in a follow up |
A few thoughts:
|
Thanks for taking a look @joseph-lozano ✨
This will appear in the change log 👍. We'll also be communicating manually to partner teams on release (they're currently waiting on it to ship 😅). The breaking change policy in Primer Brand is that we typically reserve them for API changes, or major visual changes that will require manual user acceptance testing. Example of the latter is the recent global typography update, which was wide reaching enough that we would be remiss to not highlight it. Visual changes generally don't always constitute a breaking change, otherwise we'll be issuing them all the time. On this occasion, the torch effect is actually how the card should have looked originally in dark mode (See GitHub homepage). As we tend to make a lot of visual changes to our components, we should use notices for breaking changes sparingly to avoid diluting their impact. We're also pre-v1, so we can't actually spin the
The component API is unchanged, so assume you're referring to the suggestion for color overrides? This was intentional, because the accent variable can be overridden with any color value, including E.g. With the current approach of using (the inline style would typically be set in the stylesheet, so this is just an example...) <Section style={{['--brand-color-accent-primary']: '#8E47FE'}>
{/* color will automatically be inherited across all child cards */}
<Card />
<Card />
<Card />
</Section> Hope this makes sense! |
@rezrah I checked a similar situation in a previous accessibility review and relying on OS level user setting is the safest, so I think we are good here, If we want to support and on/off toggle of this effect, my suggestion would be to have a non-motion based hover state - like a border color/opacity or internal link hover trigger only. |
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 agree with the comments from @nsolerieu and I think the torchlight effect looks great on its own. 👍
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.
Effect looks good 👍 unrelated, but also related: did we accidentally regress on the arrow animation for these cards, or did we remove it intentionally? (Arrow have always animated out to the right on hover and on focus, with the stem of the arrow appearing with that animation)
arrow-animation.mov
Update: Arrow hover animation has been fixed... cc. @tobiasahlin @nsolerieu 👇 Screen.Recording.2023-10-19.at.12.46.18.mov
New prop added to optionally disable the hover effect. Now just underlines the heading and expands the arrow.
|
Summary
Adds a new default hover effect to
Card
components in dark mode. The torchlight color respects the current, active accent color. This effect has been adapted from its web component counterpart on github.com/home.Light mode remains unchanged, as the 'torchlight' effect can only apply to
dark
pages.🔗 Try it here
Screen.Recording.2023-10-18.at.16.28.36.mov
List of notable changes:
Steps to test:
Contributor checklist:
Reviewer checklist:
Screenshots:
Torch color automatically changes based on level 2 templates based on active theme:
level.2.themes.mov