Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Updating components to use success over secondary color option #4888

Merged
merged 27 commits into from
Jun 21, 2021
Merged

Updating components to use success over secondary color option #4888

merged 27 commits into from
Jun 21, 2021

Conversation

babs20
Copy link
Contributor

@babs20 babs20 commented Jun 16, 2021 •
edited
Loading

Summary

Fixes #4882

Updating components to use euiColorSuccess over euiColorSecondary.

Badge

Added 'success' color option

Before

Screen Shot 2021-06-16 at 9 33 33 PM

After

Screen Shot 2021-06-16 at 9 38 00 PM

Expression

Added 'success' color option and changed 'success' to default color option.

Before

Screen Shot 2021-06-16 at 9 41 04 PM

After

Screen Shot 2021-06-16 at 9 40 33 PM

Progress

Change default color option to 'success'.

Before

Screen Shot 2021-06-16 at 9 51 36 PM

After

Screen Shot 2021-06-16 at 9 52 01 PM

Suggest Input Icon

Change default Icon color to 'success' for 'saved' and 'unchanged' states.

Text Color

Add 'success' color option.

Before

Screen Shot 2021-06-16 at 9 54 39 PM

After

Screen Shot 2021-06-16 at 9 55 29 PM

Tour Step Indicator

Change the indicator Icon color option to 'success'.

To-Do

  • There are a couple of components (i.e. Stat and NotificationEvent) that don't use the prop color, but use or reference the euiColorSecondary color in a similar fashion to the updated components. Not sure if these should also be changed over to euiColorSuccess.

- Need to do a final check over all the changes, and add before and after photos.

Checklist

  • Check against all themes for compatibility in both light and dark modes
  • Checked in mobile
  • Checked in Chrome, Safari, Edge, and Firefox
  • Props have proper autodocs and playground toggles
  • Added documentation
  • Checked Code Sandbox works for the any docs examples
  • Added or updated jest tests
  • Checked for breaking changes and labeled appropriately
  • Checked for accessibility including keyboard-only and screenreader modes
  • A changelog entry exists and is marked appropriately

@kibanamachine
Copy link

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@cla-checker-service
Copy link

cla-checker-service bot commented Jun 16, 2021 •
edited
Loading

💚 CLA has been signed

@babs20
Copy link
Contributor Author

babs20 commented Jun 16, 2021

❌ Author of the following commits did not sign a Contributor Agreement:
9ca630c, 84ad386, 63be299, b1843bb, 35b5c13, cfa524a, 2e3488d, 8ca5126, 78be489, fa27c29, 71c301e, 5409f23, a70af97

Please, read and sign the above mentioned agreement if you want to contribute to this project

Done.

@hetanthakkar
Copy link
Contributor

@babs20 Don't forget to mention your changes in changelog.md

@babs20 babs20 marked this pull request as ready for review June 17, 2021 02:48
@cchaos
Copy link
Contributor

cchaos commented Jun 17, 2021

🎉 Thanks @babs20 !!! I'll get our CI (Jenkins) started for you which will run our tests and get a preview built so we can review.

Jenkins, test this

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_4888/

cchaos
cchaos suggested changes Jun 17, 2021
View reviewed changes
Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

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

There are a couple of components (i.e. Stat and NotificationEvent) that don't use the prop color, but use or reference the euiColorSecondary color in a similar fashion to the updated components. Not sure if these should also be changed over to euiColorSuccess.

Yeah, I think no matter what the prop name is, if it's doing some sort of coloring based on those variables and doesn't have a success option, we should add success. You can add that to this PR or comment/create a new issue about addressing those.

All the changes look great to me. All additive, no breaking changes. Just a few small suggestions.

src/components/health/health.tsx Outdated Show resolved Hide resolved
src/components/expression/_variables.scss Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
babs20 and others added 2 commits June 17, 2021 22:16
@babs20 @cchaos
remove secondary from list
31fec21 
Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com>
@babs20 @cchaos
Update CHANGELOG.md
d5425e5 
Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com>
@babs20
Copy link
Contributor Author

babs20 commented Jun 18, 2021

Yeah, I think no matter what the prop name is, if it's doing some sort of coloring based on those variables and doesn't have a success option, we should add success. You can add that to this PR or comment/create a new issue about addressing those.

I can go ahead and add them to this PR since they are similar to the other changes.

@babs20 babs20 closed this Jun 18, 2021
@babs20 babs20 deleted the sucess_color_prop branch June 18, 2021 03:36
@babs20 babs20 restored the sucess_color_prop branch June 18, 2021 03:37
@babs20 babs20 reopened this Jun 18, 2021
@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_4888/

cchaos
cchaos suggested changes Jun 21, 2021
View reviewed changes
src/components/icon/_icon.scss Outdated Show resolved Hide resolved
@cchaos
Copy link
Contributor

cchaos commented Jun 21, 2021

Jenkins, test this

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_4888/

cchaos
cchaos approved these changes Jun 21, 2021
View reviewed changes
Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

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

🎉 Looks great! Thanks for your work on this one! Looks like we might just need a quick rebase from master and then I can merge when CI passes.

@cchaos
Copy link
Contributor

cchaos commented Jun 21, 2021

Jenkins, test this

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_4888/

@cchaos cchaos merged commit 773564a into elastic:master Jun 21, 2021
@cchaos cchaos added the deprecations Contains deprecations. Add them to the deprecations meta ticket after merge. label Jun 21, 2021
@cchaos cchaos mentioned this pull request Jun 21, 2021
Open
33 tasks
@thompsongl thompsongl mentioned this pull request Jun 24, 2021
Merged
@thompsongl thompsongl mentioned this pull request Oct 26, 2021
Merged
5 tasks
@GerardoPM GerardoPM mentioned this pull request Mar 7, 2022
Merged
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cchaos cchaos cchaos approved these changes

Assignees
No one assigned
Labels
deprecations Contains deprecations. Add them to the deprecations meta ticket after merge.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Deprecate all usages of secondary as a color type
4 participants
@babs20 @kibanamachine @hetanthakkar @cchaos