[WEB-4025] 14.7.6 - fix: explicitly set Icon size units #525
Conversation
WalkthroughThe pull request includes updates to the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
jamiehenson
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
src/core/styles/colors/Colors.stories.tsx (1)
118-122: Consider adding a comment about browser compatibility.
Since this change was specifically made to address Firefox rendering issues, it would be helpful to add a comment explaining why explicit units are necessary.
<Icon
name="icon-gui-arrow-right"
+ // Explicit px unit required for consistent rendering across browsers
size="48px"
additionalCSS="m-16"
></Icon>src/core/Pricing/PricingCards.tsx (2)
Line range hint 232-236: LGTM: Explicit px unit added to checkmark icon size.
The change from size="16" to size="16px" ensures consistent icon rendering across browsers.
Consider adding aria-hidden="true" to the checkmark icons since their meaning is conveyed through the accompanying text, unless these icons are intended to be announced by screen readers.
75-77: Consider documenting browser compatibility requirements.
Since these changes were necessitated by Firefox's stricter unit requirements, consider adding a comment in the component or documentation about the importance of explicit units for cross-browser compatibility.
Add a comment like:
// Note: Explicit 'px' units are required for consistent icon rendering across browsersAlso applies to: 232-236
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- package.json (1 hunks)
- src/core/Accordion.tsx (1 hunks)
- src/core/Expander/Expander.stories.tsx (1 hunks)
- src/core/Pricing/PricingCards.tsx (2 hunks)
- src/core/styles/colors/Colors.stories.tsx (1 hunks)
✅ Files skipped from review due to trivial changes (2)
- package.json
- src/core/Accordion.tsx
🔇 Additional comments (3)
src/core/styles/colors/Colors.stories.tsx (1)
118-122: LGTM! Icon size unit explicitly defined.
The change from implicit to explicit size units aligns with the PR's objective to fix Firefox rendering issues.
Let's verify if there are any other Icon components in this file that might need similar updates:
✅ Verification successful
Icon size units are consistently defined in this file
The verification shows only one Icon component usage in Colors.stories.tsx, and it already has the correct pixel unit format (48px). No other instances need updating in this file.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other Icon component usages in this file
rg --type tsx "Icon.*size=[\"'](\d+)[\"']" "src/core/styles/colors/Colors.stories.tsx"
Length of output: 118
Script:
#!/bin/bash
# Search for Icon components in the file without type restriction
rg "Icon.*size=" "src/core/styles/colors/Colors.stories.tsx" -A 1
Length of output: 65
Script:
#!/bin/bash
# Search for Icon component usage with more generic pattern
rg "<Icon" "src/core/styles/colors/Colors.stories.tsx" -A 3
Length of output: 164
src/core/Expander/Expander.stories.tsx (1)
143-143: LGTM! The explicit unit addition aligns with cross-browser compatibility requirements.
The change from size="24" to size="24px" ensures consistent icon rendering across browsers, particularly addressing the Firefox-specific issue where numeric values without units are interpreted differently than in Chrome.
Let's verify consistent unit usage across other Icon components in the codebase:
✅ Verification successful
The change is consistent with the codebase's icon size unit conventions
The verification confirms that all Icon size props in the codebase consistently use the "px" unit (found in Accordion.tsx, Colors.stories.tsx, PricingCards.tsx, and Expander.stories.tsx). No instances of unitless numeric values were found, indicating this change maintains the established pattern of explicit pixel units for icon sizes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining Icon size props without explicit units
# This helps ensure all Icon instances follow the same pattern
# Search for Icon size props without px units
rg 'size=["\']\d+["\']' --type tsx --type jsx
# Search for Icon size props with px units for comparison
rg 'size=["\']\d+px["\']' --type tsx --type jsx
Length of output: 93
Script:
#!/bin/bash
# Search for Icon size props without px units
rg "size=['\"]\\d+['\"]" --type tsx --type jsx
# Search for Icon size props with px units for comparison
rg "size=['\"]\\d+px['\"]" --type tsx --type jsx
Length of output: 161
Script:
#!/bin/bash
# Search for Icon components with size props without px units
rg "size=['\"]\\d+['\"]" -t ts -t js -t tsx -t jsx
# Search for Icon components with size props with px units
rg "size=['\"]\\d+px['\"]" -t ts -t js -t tsx -t jsx
Length of output: 169
Script:
#!/bin/bash
# Search for Icon components with size props without px units
rg "size=['\"]\\d+['\"]"
# Search for Icon components with size props with px units
rg "size=['\"]\\d+px['\"]"
Length of output: 532
src/core/Pricing/PricingCards.tsx (1)
75-77: LGTM: Explicit px unit added to delimiter icon size.
The change from size="20" to size="20px" aligns with the PR objective to fix icon rendering in Firefox.
Jira Ticket Link / Motivation
It was pointed out that pricing page looks like crap on Firefox. This is because of how Firefox handles values provided for inline styling - it needs greater specificity than Chrome. In this particular example, a size of "16" was interpreted as "16px" in Chrome, but as an erroneous value in Firefox, hence the massive icons.
Summary of changes
Fix the unit definitions for sizes for
Icons to fix icon rendering on Firefox.How do you manually test this?
Reviewer Tasks (optional)
Merge/Deploy Checklist
Frontend Checklist
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
PricingCardscomponent to conditionally apply margin based on thedelimiterprop.