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.

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

Add Color.toCssHexString and unit tests #8987

Merged
merged 4 commits into from
Jun 24, 2020
Merged

Add Color.toCssHexString and unit tests #8987

merged 4 commits into from
Jun 24, 2020

Conversation

ErixenCruz
Copy link
Contributor

Added Color.toCssHexString for getting the CSS hex string equivalent for a color.

@cesium-concierge
Copy link

Thank you so much for the pull request @ErixenCruz! I noticed this is your first pull request and I wanted to say welcome to the Cesium community!

The Pull Request Guidelines is a handy reference for making sure your PR gets accepted quickly, so make sure to skim that.

  • ❌ Missing CONTRIBUTORS.md entry.
  • ✔️ Signed CLA found.

Reviewers, don't forget to make sure that:

  • Cesium Viewer works.
  • Works in 2D/CV.
  • Works (or fails gracefully) in IE11.

@ErixenCruz ErixenCruz requested a review from lilleyse June 23, 2020 16:22
@mramato
Copy link
Contributor

mramato commented Jun 23, 2020

What's the use case for this? toCssColorString can be used in CSS anywhere toCssHexString could also be used, right? And it supports alpha.

@mramato
Copy link
Contributor

mramato commented Jun 23, 2020

Also, hex strings with alpha work everywhere but IE11, so I'm not sure why we would leave alpha off: https://caniuse.com/#feat=css-rrggbbaa

@hpinkos
Copy link
Contributor

hpinkos commented Jun 23, 2020

I had a use case for this. There was a 3rd party component i wanted to send a color value to that only accepted hex strings

@hpinkos
Copy link
Contributor

hpinkos commented Jun 23, 2020

+1 for using #RRGGBBAA if alpha < 1 though, instead of just ignoring it

@ErixenCruz
Copy link
Contributor Author

@hpinkos Updated with your feedback.

CHANGES.md Outdated
@@ -4,6 +4,7 @@

##### Breaking Changes :mega:

- Added `Color.toCssHexString` for getting the CSS hex string equivalent for a color.
Copy link
Contributor

Choose a reason for hiding this comment

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

This should go under the Additions section instead of the Breaking Changes section

@hpinkos
Copy link
Contributor

hpinkos commented Jun 24, 2020

@ErixenCruz Be sure to add yourself to https://github.com/CesiumGS/cesium/blob/master/CONTRIBUTORS.md under the CesiumGS section!

@ErixenCruz
Copy link
Contributor Author

I added myself in #8983. Should I add myself here, too? @hpinkos

@hpinkos
Copy link
Contributor

hpinkos commented Jun 24, 2020

@ErixenCruz yes please! I think this PR will be ready to be merged first, and we should add your name to the contributors list when this goes in

@ErixenCruz
Copy link
Contributor Author

@hpinkos Pushed commit with your feedback. Thanks.

CHANGES.md Outdated Show resolved Hide resolved
@ErixenCruz
Copy link
Contributor Author

@lilleyse Updated.

@lilleyse
Copy link
Contributor

@ErixenCruz thanks! Looks good.

@lilleyse lilleyse merged commit 365560f into master Jun 24, 2020
@lilleyse lilleyse deleted the color-CssHexString branch June 24, 2020 21:21
@nmschulte
Copy link
Contributor

This is very useful, thank you!

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.

6 participants