Skip to content

Conversation

@olaf-k
Copy link
Member

@olaf-k olaf-k commented Nov 14, 2025

Continuation of #37 - migrated from fork branch

@olaf-k olaf-k requested a review from R-Bower as a code owner November 14, 2025 10:32
@olaf-k olaf-k force-pushed the olaf/badge branch 2 times, most recently from 2fbe48f to 6c2dd9a Compare November 14, 2025 11:39
olaf-k and others added 2 commits November 14, 2025 12:59
@github-advanced-security
Copy link

This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation.

@olaf-k olaf-k requested a review from R-Bower November 18, 2025 14:45
Copy link
Contributor

@R-Bower R-Bower left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My main concerns with this component are the styles.

Size overrides

If you find yourself overriding the defaults in most of your variants, then that's probably an indicator that you should keep the styles isolated for each variant. Right now the sizes are tightly coupled to a single class with no clear benefit:

  • Base size rules assume text-badge behavior (padding, border-radius, font)
  • Icon/status/number badges use custom properties from base but override layout
  • Icon badge needs --icon-radius and --icon-size
  • Status badge needs --status-size
  • Number badge needs --count-size

Isolated colors

Interesting that you chose to couple the styles but not the colors :)

There's some color overlap between variants:

  • icon/text = identical ramp
  • status/number = similar ramp (number has persistent black/white, but the rest looks the same)

There's a lot of potential for reuse here.

I took a stab at this, give this a once-over and lmk what you think:
#61

Comment on lines 12 to 22
export type QdsNumberBadgeVariant =
| "neutral"
| "neutral-outline"
| "brand"
| "brand-outline"
| "info"
| "success"
| "warning"
| "danger"
| "persistent-black"
| "persistent-white"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the other badges, we are using emphasis for color. The design system uses emphasis for color as well. This is also true for other components. Let's keep it consistent.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the number badge mixes emphasis and variant styles, I don't mind one name or the other.

@olaf-k
Copy link
Member Author

olaf-k commented Nov 21, 2025

Interesting that you chose to couple the styles but not the colors :)

good point! thanks for the followup PR.

R-Bower
R-Bower previously approved these changes Nov 24, 2025
Signed-off-by: Ryan Bower <[email protected]>
@R-Bower R-Bower merged commit 3d6cb6f into main Nov 24, 2025
21 checks passed
@R-Bower R-Bower deleted the olaf/badge branch December 5, 2025 07:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants