fix(theme): added stripe color gradients for progress #3938
Conversation
|
@ShrinidhiUpadhyaya is attempting to deploy a commit to the NextUI Inc Team on Vercel. A member of the Team first needs to authorize it. |
🦋 Changeset detectedLatest commit: 3775ca4 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughThe pull request introduces enhancements to the Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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
Documentation and Community
|
![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 (3)
.changeset/poor-moose-double.md (1)
1-5: Enhance the changeset description for better changelog clarity.The patch version bump is appropriate for this bug fix. However, the description could be more detailed to better document the changes in the changelog.
Consider expanding the description to:
--- "@nextui-org/theme": patch --- -added stripe color gradients for progress (#1933) +fix(progress): add stripe color gradients for progress component (#1933) + +- Fixed progress component not displaying specified colors with isStriped property +- Added new gradient classes for primary, secondary, success, warning, and danger variants +- Updated background gradient colors from RGBA to HEX formatpackages/core/theme/src/components/progress.ts (1)
153-161: Maintain consistent spacing between compound variants.There are inconsistent empty lines between the compound variants. Consider removing the extra empty lines at 153 and 161 to maintain consistent spacing throughout the configuration.
packages/core/theme/src/plugin.ts (1)
220-231: LGTM! The gradient definitions effectively address the Progress component color issue.The implementation provides a comprehensive set of stripe gradients for different color variants, effectively fixing issue #1933. The consistent 45-degree angle and 25% color stops create a visually appealing striped pattern.
Consider adding comments to document the color pairs and their use cases. This would help future maintainers understand the color relationships and make updates easier.
Example:
backgroundImage: { + // Default: Neutral gray stripes (zinc-700) "stripe-gradient-default": "linear-gradient(45deg, #3f3f46 25%, transparent 25%, transparent 50%, #3f3f46 50%, #3f3f46 75%, transparent 75%, transparent)", + // Primary: Blue stripes (darker -> lighter) "stripe-gradient-primary": "linear-gradient(45deg, #004493 25%, #006FEE 25%, #006FEE 50%, #004493 50%, #004493 75%, #006FEE 75%, #006FEE)", // ... (document remaining color pairs)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
- .changeset/poor-moose-double.md (1 hunks)
- packages/core/theme/src/components/progress.ts (2 hunks)
- packages/core/theme/src/plugin.ts (1 hunks)
- packages/core/theme/src/utils/tw-merge-config.ts (1 hunks)
🔇 Additional comments (4)
packages/core/theme/src/utils/tw-merge-config.ts (1)
14-21: LGTM! The gradient class configuration looks complete.The changes correctly implement all the required color variants for the striped progress bars, following a consistent naming convention.
Let's verify that these gradients are properly defined in the theme configuration:
✅ Verification successful
Let me explain the new approach:
- First command searches for any files containing "stripe-gradient" to locate relevant configuration files
- Second command looks for background image configurations which might define these gradients using a broader pattern match
This should help us verify if the gradients are properly defined in the theme configuration.
The previous search revealed that gradient definitions are likely in
plugin.ts, but we only saw the default gradient. Let's verify if all the required variants (primary, secondary, success, warning, danger) are properly defined.
The previous search had a regex error, but we found important information in progress.ts showing that all stripe gradient classes are being used in compound variants. Let's examine plugin.ts directly to verify if all these gradients are properly defined in the theme configuration.
All gradient definitions are properly configured and match the tw-merge config
The verification is complete and successful. The code shows that:
- All stripe gradients are properly defined in
plugin.tsunderbackgroundImageconfiguration- Each variant (default, primary, secondary, success, warning, danger) has its corresponding gradient definition
- The gradients are properly used in the Progress component's compound variants
- The naming convention in tw-merge-config.ts matches exactly with the actual implementations
Points of verification:
- ✓ Gradient classes in tw-merge-config.ts match the theme definitions
- ✓ Each gradient has a proper linear-gradient implementation with appropriate color values
- ✓ The Progress component correctly utilizes these gradients in its compound variants
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that all gradient classes have corresponding definitions in the theme configuration # Test: Search for gradient definitions in theme files # Expect: Each bg-stripe-gradient-* class should have a corresponding gradient definition rg -A 1 "backgroundImage.*stripe-gradient-" packages/core/theme/src/Length of output: 70
Script:
#!/bin/bash # Let's try a broader search for gradient-related configurations # First, let's check theme configuration files rg -l "stripe-gradient" packages/core/theme/src/ # Then, let's look for any backgroundImage or background-image configurations rg -A 2 "backgroundImage|background-image" packages/core/theme/src/Length of output: 760
Script:
#!/bin/bash # Let's check the content of plugin.ts to see all gradient definitions rg -A 10 "backgroundImage: {" packages/core/theme/src/plugin.ts # Also check progress.ts since it was found in the previous search cat packages/core/theme/src/components/progress.tsLength of output: 4469
Script:
#!/bin/bash # Let's try a different approach to search for backgroundImage in plugin.ts cat packages/core/theme/src/plugin.tsLength of output: 10850
packages/core/theme/src/components/progress.ts (2)
95-95: LGTM! Clear and consistent naming convention.The renamed class
bg-stripe-gradient-defaultfollows a more explicit naming pattern that aligns well with other color variants.
132-168: Verify visual appearance of all color variants.The implementation looks correct, but please ensure that all color variants have been tested with the
isStripedproperty, especially edge cases:
- Color transitions during progress updates
- Appearance at different progress values (0%, 50%, 100%)
- Interaction with different size variants
- Interaction with different radius variants
✅ Verification successful
Let me check the gradient definitions in the theme files to ensure they are properly defined and consistent.
Stripe gradients implementation is correctly configured and consistent
The verification shows that:
- All color variants (default, primary, secondary, success, warning, danger) have proper gradient definitions in
plugin.ts- The gradients use consistent color pairs for each variant, creating a proper striped effect
- The gradient utilities are correctly registered in the Tailwind merge configuration
- The implementation in
progress.tscorrectly references these gradient classesThe original review comment's request for visual testing remains valid, but the technical implementation is sound and complete.
Points to manually verify during testing:
- Progress bar animations with the new gradients
- Color contrast between stripe pairs in each variant
- Gradient scaling across different progress bar sizes
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that all color variants are properly exported and accessible # Check if all color variants are consistently defined across theme files rg -l "stripe-gradient-(default|primary|secondary|success|warning|danger)"Length of output: 206
Script:
#!/bin/bash # Check gradient definitions in theme files echo "=== Checking plugin.ts ===" rg "stripe-gradient-" packages/core/theme/src/plugin.ts -A 2 -B 2 echo -e "\n=== Checking tw-merge-config.ts ===" rg "stripe-gradient-" packages/core/theme/src/utils/tw-merge-config.ts -A 2 -B 2Length of output: 1665
packages/core/theme/src/plugin.ts (1)
220-231: Verify color contrast ratios for accessibility.The gradient color pairs should maintain sufficient contrast for accessibility. Let's verify the contrast ratios between the color pairs.
| { | ||
| color: "primary", | ||
| isStriped: true, | ||
| class: { | ||
| indicator: "bg-stripe-gradient-primary bg-[length:1.25rem_1.25rem]", | ||
| }, | ||
| }, | ||
| { | ||
| color: "secondary", | ||
| isStriped: true, | ||
| class: { | ||
| indicator: "bg-stripe-gradient-secondary bg-[length:1.25rem_1.25rem]", | ||
| }, | ||
| }, | ||
| { | ||
| color: "success", | ||
| isStriped: true, | ||
| class: { | ||
| indicator: "bg-stripe-gradient-success bg-[length:1.25rem_1.25rem]", | ||
| }, | ||
| }, | ||
|
|
||
| { | ||
| color: "warning", | ||
| isStriped: true, | ||
| class: { | ||
| indicator: "bg-stripe-gradient-warning bg-[length:1.25rem_1.25rem]", | ||
| }, | ||
| }, | ||
|
|
||
| { | ||
| color: "danger", | ||
| isStriped: true, | ||
| class: { | ||
| indicator: "bg-stripe-gradient-danger bg-[length:1.25rem_1.25rem]", | ||
| }, | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider extracting repeated background size to reduce duplication.
The bg-[length:1.25rem_1.25rem] is repeated across all color variants. Consider extracting this to a shared class in your Tailwind configuration to follow the DRY principle.
compoundVariants: [
{
color: "primary",
isStriped: true,
class: {
- indicator: "bg-stripe-gradient-primary bg-[length:1.25rem_1.25rem]",
+ indicator: "bg-stripe-gradient-primary stripe-bg-size",
},
},
// ... repeat for other colors
],Then define the shared class in your Tailwind configuration:
// tailwind.config.js
module.exports = {
theme: {
extend: {
utilities: {
'.stripe-bg-size': {
'background-size': '1.25rem 1.25rem'
}
}
}
}
}
ShrinidhiUpadhyaya
changed the title
wingkwong
changed the title
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
wingkwong
requested review from
wingkwong
and removed request for
jrgarciadev
Yes the colors are same as NextUI theme. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
packages/core/theme/src/plugin.ts (1)
155-156: LGTM! Consider improving readability.The
createStripeGradientfunction correctly implements the stripe pattern using linear gradients and properly integrates with the theme system using HSL colors.Consider breaking down the long string into multiple lines for better readability:
- const createStripeGradient = (stripeColor: string, backgroundColor: string) => - `linear-gradient(45deg, hsl(var(--${prefix}-${stripeColor})) 25%, hsl(var(--${prefix}-${backgroundColor})) 25%, hsl(var(--${prefix}-${backgroundColor})) 50%, hsl(var(--${prefix}-${stripeColor})) 50%, hsl(var(--${prefix}-${stripeColor})) 75%, hsl(var(--${prefix}-${backgroundColor})) 75%, hsl(var(--${prefix}-${backgroundColor})))`; + const createStripeGradient = (stripeColor: string, backgroundColor: string) => ` + linear-gradient( + 45deg, + hsl(var(--${prefix}-${stripeColor})) 25%, + hsl(var(--${prefix}-${backgroundColor})) 25%, + hsl(var(--${prefix}-${backgroundColor})) 50%, + hsl(var(--${prefix}-${stripeColor})) 50%, + hsl(var(--${prefix}-${stripeColor})) 75%, + hsl(var(--${prefix}-${backgroundColor})) 75%, + hsl(var(--${prefix}-${backgroundColor})) + )`;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- packages/core/theme/src/components/progress.ts (2 hunks)
- packages/core/theme/src/plugin.ts (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/core/theme/src/components/progress.ts
🔇 Additional comments (2)
packages/core/theme/src/plugin.ts (2)
222-224: LGTM! Background size is well-defined.The stripe size of 1.25rem provides a good visual balance and uses appropriate rem units for accessibility.
226-231: LGTM! Verify color contrast ratios.The gradient definitions correctly implement all required color variants, addressing issue #1933. The use of -200 variants for stripes provides good visual distinction.
Let's verify that the color pairs meet WCAG contrast guidelines:
Closes #1933
📝 Description
bg-stripe-gradienttobg-stripe-gradient-defaultbg-stripe-gradient-defaultto#3f3f46#3f3f46,transparent#004493,#006FEE#6020A0,#9353d3#0E793C,#17c964#936316,#f5a524#920B3A,#f31260⛳️ Current behavior (updates)
🚀 New behavior
💣 Is this a breaking change (Yes/No):
📝 Additional Information
pnpm buildto see the new colors.Summary by CodeRabbit
New Features
Bug Fixes