-
-
Notifications
You must be signed in to change notification settings - Fork 571
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
Support the class
attribute for hero action link buttons
#2253
Support the class
attribute for hero action link buttons
#2253
Conversation
🦋 Changeset detectedLatest commit: 31b8ed3 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
✅ Deploy Preview for astro-starlight ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
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.
Looks good @HiDeoo — I’m curious about one detail, but happy with the code.
))} | ||
{actions.map( | ||
({ attrs: { class: className, ...attrs } = {}, icon, link: href, text, variant }) => ( | ||
<LinkButton {href} {variant} icon={icon?.name} class:list={[className]} {...attrs}> |
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.
Huh, interesting! Do you know why this extra work is needed? Because Astro already sets a class
and uses that instead of the one spread in from attrs
? Feels like maybe something we should report to the team.
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.
Good question, I always assumed this was the expected pattern to use with our scopedStyleStrategy
when you want to accept a class that you would pass down to a child and the component also get a scoped class. Using class
would result with one being overwritten by the other so you would have to use class:list
. Thinking about it, I guess this may be handled automatically, never thought about it, could be a good question to ask 🤔
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.
Asked on Discord, but I think we can still make this change as a fix for now!
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.
Thanks for the fix @HiDeoo!
This PR fixes an issue reported on Discord where trying to use the
class
attribute in hero action link buttons was not working as expected.The link button generated with the following frontmatter would not include the
TEST_CLASS_WARNING_REMOVE_ME
class: