feat(badge): add href and icon props - #734
Conversation
|
/bonk review |
commit: |
Docs PreviewCommit: |
|
Thanks for the PR! The I have a few concerns, mainly around the significant test removals that don't appear to be justified by the feature changes. Blocking1. Unjustified test removalsA large number of existing tests were removed without explanation:
These tests verified behavior that still exists and would still pass with your changes. Removing them reduces coverage and makes the codebase more fragile. Please either restore them or explain in the PR description why each removal is necessary. Suggestions2.
|
|
Posted a review on PR #734. SummaryBlocking concern: The PR removes ~8 existing tests without justification. Tests covering Other feedback given:
Positive notes: Changeset is appropriate, |
|
/bonk re-review and approve please |
There was a problem hiding this comment.
Previous concerns have been addressed. The href prop was removed from Badge in favor of wrapping with Link, which is a cleaner API that avoids prop-creep and is consistent with the rest of the library. All existing badge tests remain intact, and the new icon prop is well-covered with unit tests and type tests. The discriminated union for FilledBadgeProps | DotBadgeProps correctly prevents passing icon to dot badges. Docs demos and visual test fixtures are a nice addition.
|
Approved PR #734. The previous blocking concern about test removals is resolved—all existing tests remain intact. The decision to drop the |























































































This PR adds the
iconandhrefprops to theBadgecomponent:This allows us to finally migrate the badges in the worker cards away from the bespoke implementation: