-
Notifications
You must be signed in to change notification settings - Fork 1
feat: update progress bar default fill to horizontal gradient #515
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
base: main
Are you sure you want to change the base?
Conversation
- Replace solid red (#FF3E14) with gradient (#FDA4AF → #FF3E14) - Add gradient colors as CSS variables for future flexibility - Preserve existing CSS variable fallback system for custom colors - Only affects default variant, custom colors remain unchanged DSS-1497 Co-Authored-By: Cameron Simony <cameron.simony@kajabi.com> Co-Authored-By: cameron.simony@kajabi.com <cameron.simony@gmail.com>
Original prompt from cameron.simony |
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
✅ Deploy Preview for pine-design-system ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
…s value - Change :host([fill-color]) to :host([fill-color]:not([fill-color=""])) - This prevents empty fill-color attribute from overriding the gradient - Ensures default progress bar shows gradient while custom colors still work Co-Authored-By: cameron.simony@kajabi.com <cameron.simony@gmail.com>
| --progress-gradient-start: #fda4af; | ||
| --progress-gradient-end: #ff3e14; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These color values should use tokens and not hex values.
|
aside @cameronsimony, please ensure these adjustments are also made in Figma to keep our design system consistent. Since progress bars are used throughout the application, it's probably a good idea to proactively communicate this change to all teams so they don't flood us with bug reports thinking something's broken. It's a minor code change, but it could be jarring in locations where other progress bars on the page do not use gradients. |
- Use --pine-color-red-300 for gradient start (lighter color) - Use --pine-color-brand for gradient end (brand color) - Addresses GitHub comment about using design tokens instead of hex values Co-Authored-By: cameron.simony@kajabi.com <cameron.simony@gmail.com>
QuintonJason
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
aside
@cameronsimony Rejecting since a change is occurring that isn't represented in the corresponding mercury component design file: https://www.figma.com/design/CC1YmaGKHnsvB28yLY9mEH/%E2%9D%96-Mercury-components?m=auto&node-id=737-22323
|
Updated the figma side here I'll communicate to the teams before a merge 👍 |
| :host([fill-color]:not([fill-color=""])) progress::-webkit-progress-value { | ||
| background: var(--color-progress-fill); | ||
| } | ||
|
|
||
| :host([fill-color]:not([fill-color=""])) progress::-moz-progress-bar { | ||
| background: var(--color-progress-fill); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could these be comma-separated and combined into one?
- Consolidate custom color override selectors into comma-separated rule - Improves code maintainability while preserving functionality - Addresses GitHub comment from pixelflips about combining selectors Jira: DSS-1497 Co-Authored-By: cameron.simony@kajabi.com <cameron.simony@gmail.com>
- Change gradient end color from brand to mercury-500 - Addresses feedback about incorrect gradient color specification - Maintains existing functionality with correct design tokens Jira: DSS-1497 Co-Authored-By: cameron.simony@kajabi.com <cameron.simony@gmail.com>
| :host { | ||
| --color-progress-fill: var(--pine-color-brand); | ||
| --progress-gradient-start: var(--pine-color-red-300); | ||
| --progress-gradient-end: var(--pine-color-mercury-500); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use var(--pine-color-brand) here
- Address GitHub comment from pixelflips to use brand token - More semantically correct than direct mercury-500 reference - Maintains same visual appearance while using proper token Jira: DSS-1497 Co-Authored-By: cameron.simony@kajabi.com <cameron.simony@gmail.com>
pixelflips
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Please do not merge until @QuintonJason approves as well.
QuintonJason
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cameronsimony @pixelflips Requesting changes because this fix feels too narrowly scoped. Since this introduces a gradient token into the system, we should take a system-wide approach to defining and using gradient components. The progress bar should consume this shared token rather than introducing a one-off implementation.
We’ll also need to build out the gradient options in the design files so we can explore and align on all possible variations before locking in an implementation.
Additionally, the current code does not allow for a custom gradient, which is a limitation we should address to maintain flexibility.
|
aside @QuintonJason i agree. any broader gradient implementation will need a comprehensive plan. for this specific change though, we could move forward with a quick follow-up to add a component-level token, but that depends on the scope of what we're trying to achieve. @cameronsimony could you clarify the intended scope of these changes? if this is solely updating the default progress bar styling, we can proceed and follow up immediately with the component token. however, if the plan includes multiple variants or custom gradient support, that would require a more extensive project approach. quinton raises some important considerations, so we'd appreciate clarification on the design goals before merging. |
|
@cameronsimony have you had a chance to review the comments above? |
Description
This PR updates the default progress bar fill color from solid red (#FF3E14) to a horizontal gradient (#FDA4AF → #FF3E14) in the Pine design system, as specified in DSS-1497.
Changes Made
--progress-gradient-startand--progress-gradient-end) to enable future customization::-webkit-progress-valueand::-moz-progress-bar) to usebackgroundwith gradient fallback instead ofbackground-colorThe implementation uses CSS variable fallback logic:
var(--color-progress-fill, linear-gradient(to right, var(--progress-gradient-start), var(--progress-gradient-end)))to maintain backward compatibility.Fixes DSS-1497
Type of change
How Has This Been Tested?
Test Configuration:
Critical Review Areas
Please pay special attention to:
--color-progress-fillstill overrides the gradientvar(--color-progress-fill, linear-gradient(...))behaves as expectedChecklist:
Link to Devin run: https://app.devin.ai/sessions/f76dcde5eca0423a9fcdba1671311c27
Requested by: Cameron Simony (@cameronsimony)
Note: This PR is part of a coordinated update across both Pine and Sage design systems. A corresponding PR for Sage will be created separately.