Skip to content

Conversation

@kaelig
Copy link
Contributor

@kaelig kaelig commented Oct 7, 2020

WHY are these changes introduced?

This will fix an issue where a home card uses imperative language to describe the color prop's possible values in this polaris-next fork of ProgressBar in web.

This forked ProgressBar is used in two Home Cards, one of them being:

Screen Shot 2020-10-07 at 12 22 37 PM

I couldn't reproduce the other one, but the code shows it uses a "green" color:

https://github.com/Shopify/web/blob/master/app/sections/Home/HomeIndex/components/Onboarding/components/OnboardingFooter/components/OnboardingProgressBar/OnboardingProgressBar.tsx#L32

WHAT is this pull request doing?

Adds a new prop using the new color system, that will help us remove this fork from polaris-next.

How to 🎩

🖥 Local development instructions
🗒 General tophatting guidelines
📄 Changelog guidelines

Check out the new ProgressBar example in Storybook.

🎩 checklist

  • Tested on mobile
  • Tested on multiple browsers
  • Tested for accessibility
  • Updated the component's README.md with documentation changes
  • Tophatted documentation changes in the style guide
  • For visual design changes, pinged one of @ HYPD, @ mirualves, @ sarahill, or @ ry5n to update the Polaris UI kit

@kaelig kaelig requested review from alex-page and kyledurand October 7, 2020 21:44
Comment on lines 43 to 46
.colorHighlight {
--p-progressbar-background: var(--p-surface-neutral, #{color('sky')});
--p-progressbar-indicator: var(--p-border-highlight, #{color('teal')});
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure component-specific custom properties are the right way to fix this, guidance would be appreciated.

@github-actions
Copy link
Contributor

github-actions bot commented Oct 7, 2020

🟢 This pull request modifies 4 files and might impact 1 other files.

Details:
All files potentially affected (total: 1)
📄 UNRELEASED.md (total: 0)

Files potentially affected (total: 0)

🎨 src/components/ProgressBar/ProgressBar.scss (total: 1)

Files potentially affected (total: 1)

🧩 src/components/ProgressBar/ProgressBar.tsx (total: 0)

Files potentially affected (total: 0)

📄 src/components/ProgressBar/README.md (total: 0)

Files potentially affected (total: 0)

@kaelig
Copy link
Contributor Author

kaelig commented Oct 8, 2020

Note: This isn't urgent, and will require a chat with designers to agree on whether this should make it into Polaris and if the colors chosen are appropriate.

Copy link
Member

@kyledurand kyledurand left a comment

Choose a reason for hiding this comment

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

I like this approach 👍 Just make sure to change the prop description

@alex-page alex-page force-pushed the master branch 2 times, most recently from 4eab9e5 to 9e09ba7 Compare January 15, 2021 05:17
Base automatically changed from master to main January 21, 2021 15:38
@kaelig
Copy link
Contributor Author

kaelig commented Feb 3, 2021

This last merge makes this change compatible with the default new design language! I'll ship it soon.

@kaelig
Copy link
Contributor Author

kaelig commented Feb 3, 2021

FYI @whizkydee, it'd be great if you could make time to update the OnboardingTaskCard component over the next few weeks.

@github-actions
Copy link
Contributor

github-actions bot commented May 7, 2021

size-limit report

Path Size
cjs 141.61 KB (+0.04% 🔺)
esm 95.45 KB (+0.06% 🔺)
esnext 138.52 KB (+0.11% 🔺)
css 33.66 KB (+0.18% 🔺)

@kaelig kaelig merged commit 2d710ae into main May 8, 2021
@kaelig kaelig deleted the progress-bar-color-prop branch May 8, 2021 05:58
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