-
Notifications
You must be signed in to change notification settings - Fork 100
chore(bling): update to SHINE styling #2013
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: beta
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: cb7f683 The changes in this PR will be included in the next version bump. 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 stacks-svelte ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for stacks ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
giamir
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.
This component looks great @dancormier. ❤️🎉
I was lurking around and I wonder if we should add in the acceptance criteria of our stories also the documentation of breaking changes. I know that there is a ticket in the backlog to create a migration doc (v2 to v3). Maybe it would make sense to fill up that document as we go so that our memory is fresh on the breaking changes we made from the point of view of the consumers.
Also, a friendly reminder to add the changesets for this PR. I think this is the first time we’ll have two changesets for two different libraries in the same PR. In any case, npx changeset should guide us through creating these slightly more complex changesets. 🙂
CGuindon
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.
giamir
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.
Looks good @dancormier. I have only left a couple of minor NIT comments.
I noticed that the following components depending on Bling are now somewhat broken:
- Badge
- Topbar
- UserCard
- Table
I guess this is on purpose and we will take care of them on follow up tickets. Correct?
Thanks for taking care of this one. ❤️
@giamir Updates to consuming components' styles should usually be left for when those components are being updated to SHINE styles, but I think we should generally update any markup when there are breaking changes like there are here with bling. I've updated the bling markup in those other components' docs pages and any relevant tests to match the expected markup. I've also updated the migration guide to mention the markup changes. If nothing else, these doc updates just make things look much nicer while we wait to update the other components 🙂
My pleasure and thank you for the review! |
giamir
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.
Looks good Dan. Thanks for the updates. The only thing missing is the link to the svelte component (svelte badge) in the Bling doc page (in the frontmatter svelte: ...). Apart from that looks good to be merged from my end. 🙂
CGuindon
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.
Whoops! Good catch @CGuindon. I've updated the |













SPARK-58
https://deploy-preview-2013--stacks.netlify.app/product/components/bling/
TODO
filledmodifiersizemodifiers (sm,lg, default)activityvariantrepvariantblings-blingclassBlingfilledbool propsizestring propactivity,rep, null (default) totypepropchildrensnippet propFollow up
We'll need to follow up on this PR once a few other changes have been made to the system:
activityvariant will need to be changed topinkcolors once addedThe docs
I have not put too much consideration into the docs. @CGuindon if you have an idea of how these should be updated, let me know and I'll add those changes.
Update Mon Nov 10, 2025: I've updated the docs per the Figma as suggested.