[WEB-4025] Assert presence of units in Icon size prop values #527
Conversation
WalkthroughThe changes involve modifications to the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
🪧 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
src/core/Icon/types.ts (1)
20-24: Consider enhancing type documentation and coverage.
While the current implementation covers the most common units, consider:
- Adding JSDoc comments explaining the purpose and usage of this type
- Including additional valid CSS units like
vh,vw, and%if they might be useful in your context
+/**
+ * Represents valid size values for Icon components.
+ * Enforces the presence of units to prevent browser compatibility issues.
+ * @example
+ * - "16px"
+ * - "1.5rem"
+ * - "calc(100% - 20px)"
+ */
export type IconSize =
| `${number}px`
| `${number}em`
| `${number}rem`
+ | `${number}vh`
+ | `${number}vw`
+ | `${number}%`
| `calc(${string})`;src/core/Icon.tsx (1)
Line range hint 1-38: Consider adding runtime validation for size values.
Given the history of browser compatibility issues with icon sizes (ref: PR #525), consider adding runtime validation to ensure size values always include valid units. This could prevent similar issues in the future, especially for non-TypeScript consumers.
Suggestions:
- Add a runtime validator function for size values
- Add JSDoc comments documenting supported units and browser compatibility notes
- Consider adding a set of predefined size constants to encourage consistent usage
Would you like me to provide an example implementation of these suggestions?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- src/core/Icon.tsx (1 hunks)
- src/core/Icon/EncapsulatedIcon.tsx (2 hunks)
- src/core/Icon/types.ts (1 hunks)
🔇 Additional comments (7)
src/core/Icon/types.ts (1)
20-24: LGTM! The IconSize type effectively enforces proper units.
The type implementation using template literals ensures that size values always include units, preventing the Firefox-related issues mentioned in the PR objectives.
src/core/Icon.tsx (3)
3-3: LGTM: Import statement updated correctly.
The addition of IconSize import aligns with the PR objective of adding type safety to icon size values.
Line range hint 16-16: Verify type compatibility of default size value.
Please ensure that the default value "0.75rem" matches the IconSize type definition. Consider adding a type assertion or constant to make this relationship explicit.
Let's check the IconSize type definition:
✅ Verification successful
Default size value "0.75rem" is compatible with IconSize type
The type definition in src/core/Icon/types.ts shows that IconSize accepts string values in the format ${number}rem, making "0.75rem" a valid value. The implementation correctly uses this typed value directly in the style object for width and height properties.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for IconSize type definition
ast-grep --pattern 'type IconSize = $_'
Length of output: 81
Script:
#!/bin/bash
# Let's try different approaches to find IconSize type definition
# Search for type definition in the same file
rg -A 5 "type IconSize" src/core/Icon.tsx
# Search for exported type definition
rg -A 5 "export.*type IconSize"
# Search for any usage of IconSize
rg "IconSize"
# Look at the entire Icon.tsx file to understand the context
cat src/core/Icon.tsx
Length of output: 1925
9-9: Verify impact of type change on existing consumers.
The type change from string to IconSize improves type safety but could potentially break existing implementations.
Let's check for any existing string literals being passed to the size prop:
✅ Verification successful
Type change from string to IconSize is safe for existing consumers
The change from string to IconSize type is non-breaking as:
- The
IconSizetype allows all current usage patterns found in the codebase (px, em, rem, calc values) - All existing usages in the codebase follow the correct format:
- "32px", "1rem", "1.5rem", "2.5rem", "3rem" etc.
- The new type actually matches the existing usage patterns, providing better type safety without breaking changes
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for Icon component usage with string literals in size prop
ast-grep --pattern 'Icon $$$size="$_"'
# Search for Icon component usage with numeric literals in size prop
ast-grep --pattern 'Icon $$$size={$_}'
Length of output: 81
Script:
#!/bin/bash
# Let's try a more comprehensive search for Icon usage patterns
# Search for any Icon component usage with size prop
rg -U "Icon.*size.*" -A 2
# Also search for potential imports of Icon to ensure we're looking at the right component
rg "import.*Icon"
# And let's check the IconSize type definition to understand valid values
fd -e ts -e tsx | xargs rg "type IconSize|enum IconSize"
Length of output: 14071
Script:
#!/bin/bash
# Let's check the IconSize type definition content
cat src/core/Icon/types.ts
# Also search for any numeric values being passed to size prop
rg -U "size=\{[0-9]+" -A 1
Length of output: 673
src/core/Icon/EncapsulatedIcon.tsx (3)
5-5: LGTM: Type safety improvements
The change from string to IconSize type enhances type safety by ensuring proper unit inclusion in size values, which aligns with the PR objective to prevent browser compatibility issues.
Also applies to: 11-11
40-40: LGTM: Clean implementation of size values
The size values are correctly applied to create the visual hierarchy:
- Outer div maintains original size
- Inner div uses computed height
- Icon uses computed size
Also applies to: 44-44, 46-46
Line range hint 1-52: Verify EncapsulatedIcon usage patterns
Let's ensure all usages of EncapsulatedIcon are compatible with the new size computation logic.
✅ Verification successful
Based on the search results, I can see that EncapsulatedIcon is used in ProductTile.tsx and Icon.stories.tsx, and none of these usages explicitly set the size prop, meaning they rely on the default value of "40px". The new size computation logic in EncapsulatedIcon maintains backward compatibility by:
- Using a default size of "40px"
- Properly handling pixel values with the
-12pxand-2pxadjustments - Supporting the new
iconSizeprop as an optional parameter
No compatibility issues found with EncapsulatedIcon usage patterns
The changes to size computation are safe as they maintain backward compatibility with existing usage patterns. All current implementations will continue to work as expected with the default size value.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for EncapsulatedIcon usage patterns
echo "Checking EncapsulatedIcon usage patterns..."
rg "EncapsulatedIcon" -A 3 -B 1
# Look for potential size prop values
echo "Checking size prop values..."
ast-grep --pattern '<EncapsulatedIcon $$$size={$_}$$$>'
Length of output: 4142
jamiehenson
force-pushed
the
assert-icon-size-units
branch
from
9967684 to
723446c
Compare
jamiehenson
force-pushed
the
assert-icon-size-units
branch
from
723446c to
e2bcb3f
Compare
| @@ -16,3 +16,9 @@ export type IconName = | |||
| | (typeof iconNames.other)[number] | |||
| | (typeof iconNames.tech)[number] | |||
| | (typeof iconNames.product)[number]; | |||
|
|
|||
| export type IconSize = | |||
Jira Ticket Link / Motivation
Follows on from this, which fixes the most obvious Icon problems in Firefox by making sure proper units are provided with values given to
sizeonIcon.This PR adds some safeguards in to assert this in future, by adding a type to
sizeonIcon, as well as fixing a subsequent, smaller issue withEncapsulatedIconrelating to this.Example:
<Icon size={16} />will not work, but<Icon size='16px' />will.Summary of changes
How do you manually test this?
Reviewer Tasks (optional)
Merge/Deploy Checklist
Frontend Checklist
Summary by CodeRabbit
Summary by CodeRabbit
New Features
IconSizetype for more flexible icon size definitions.EncapsulatedIconcomponent to improve icon size handling and type safety.Bug Fixes
sizeproperty in theIconcomponent for better type alignment.Documentation