Skip to content
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

[PM-16102] Add min width on interactive badges #12514

Merged
merged 2 commits into from
Dec 23, 2024
Merged

Conversation

vleague2
Copy link
Contributor

@vleague2 vleague2 commented Dec 20, 2024

🎟️ Tracking

PM-16102

📔 Objective

This PR adds a min-width of 2.5rem to interactive badges -- button and a elements. This will make it easier to click a button that would otherwise have a smaller click box.

📸 Screenshots

Before After
Screenshot 2024-12-20 at 2 23 00 PM Screenshot 2024-12-20 at 2 25 45 PM

⏰ Reminders before review

  • Contributor guidelines followed
  • All formatters and local linters executed and passed
  • Written new unit and / or integration tests where applicable
  • Protected functional changes with optionality (feature flags)
  • Used internationalization (i18n) for all UI strings
  • CI builds passed
  • Communicated to DevOps any deployment requirements
  • Updated any necessary documentation (Confluence, contributing docs) or informed the documentation team

🦮 Reviewer guidelines

  • 👍 (:+1:) or similar for great changes
  • 📝 (:memo:) or ℹ️ (:information_source:) for notes or general info
  • ❓ (:question:) for questions
  • 🤔 (:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion
  • 🎨 (:art:) for suggestions / improvements
  • ❌ (:x:) or ⚠️ (:warning:) for more significant problems or concerns needing attention
  • 🌱 (:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt
  • ⛏ (:pick:) for minor or nitpick changes

@vleague2 vleague2 requested a review from a team as a code owner December 20, 2024 19:26
Copy link
Contributor

github-actions bot commented Dec 20, 2024

Logo
Checkmarx One – Scan Summary & Detailsae7504b8-424e-4d63-9592-65b757fe0e33

New Issues

Severity Issue Source File / Package Checkmarx Insight
HIGH CVE-2024-11395 Npm-electron-33.2.1 Vulnerable Package

Copy link

codecov bot commented Dec 20, 2024

Codecov Report

Attention: Patch coverage is 0% with 1 line in your changes missing coverage. Please review.

Project coverage is 33.78%. Comparing base (0619ef5) to head (ddca3c6).
Report is 2 commits behind head on main.

✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
libs/components/src/badge/badge.component.ts 0.00% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main   #12514   +/-   ##
=======================================
  Coverage   33.78%   33.78%           
=======================================
  Files        2912     2912           
  Lines       90693    90693           
  Branches    17151    17151           
=======================================
  Hits        30641    30641           
  Misses      57666    57666           
  Partials     2386     2386           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@willmartian willmartian self-requested a review December 20, 2024 19:42
@@ -65,7 +65,7 @@ export class BadgeComponent implements FocusableElement {
"disabled:tw-cursor-not-allowed",
]
.concat(styles[this.variant])
.concat(this.hasHoverEffects ? hoverStyles[this.variant] : [])
.concat(this.hasHoverEffects ? [...hoverStyles[this.variant], "tw-min-w-10"] : [])
Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, this is fine because I don't want to raise the scope of this ticket, but...

This is a prime example of Tailwind implicitly encouraging weird / non-performant workarounds to simple CSS functionality vs a simple :is(a,button) selector. yells at cloud

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, we could write that here instead. But we would also need to write it for every style in hoverStyles. Also, why do we have an array of classes to begin with. ☁️ ☁️ ☁️ 👴

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah.... I think it's worth revisiting how we are structuring css for these components

@vleague2 vleague2 merged commit 395258d into main Dec 23, 2024
73 of 74 checks passed
@vleague2 vleague2 deleted the ds/pm-16102/badge-width branch December 23, 2024 17:08
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