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

Adjust Color::ALICE_BLUE #9740

Closed
wants to merge 2 commits into from
Closed

Conversation

CleanCut
Copy link
Member

Objective

  • The Color::ALICE_BLUE constant is currently an off-white color, which is confusing when there is an actual Alice Blue shade that is well-known to be light blue.

Solution

  • Change the color to match the shade defined in the Wikipedia article. I used just enough precision in the floating point values so that the math would result in the matching u8 value in the wikipedia article if multiplied by 255.

Changelog

This section is optional. If this was a trivial fix, or has no externally-visible impact, you can delete this section.

  • The Color::ALICE_BLUE constant is now the correct shade of blue, rather than an off-white shade.

@CleanCut CleanCut changed the title Adjust alice blue color Adjust Color::ALICE_BLUE Sep 10, 2023
@alice-i-cecile alice-i-cecile added C-Bug An unexpected or incorrect behavior A-Rendering Drawing game state to the screen M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide labels Sep 10, 2023
@github-actions
Copy link
Contributor

It looks like your PR is a breaking change, but you didn't provide a migration guide.

Could you add some context on what users should update when this change get released in a new version of Bevy?
It will be used to help writing the migration guide for the version. Putting it after a ## Migration Guide will help it get automatically picked up by our tooling.

@IceSentry
Copy link
Contributor

IceSentry commented Sep 10, 2023

The current color is based on https://www.w3schools.com/colors/color_tryit.asp?color=AliceBlue

The wikipedia article also links this https://www.w3.org/TR/css-color-3/#svg-color which contradicts the value present in the wikipedia page

@MrGVSV
Copy link
Member

MrGVSV commented Sep 10, 2023

The wikipedia article also links this https://www.w3.org/TR/css-color-3/#svg-color which contradicts the value present in the wikipedia page

I think the Wikipedia does refer to the same color. It separates Alice Blue into two colors. I’m pretty sure the one we're using currently is based on the web version (which is what I’d expect).

@IceSentry
Copy link
Contributor

I think the Wikipedia does refer to the same color

Oh, yeah I just looked at the first one and saw it was different. Didn't notice it listed a second one 😅

@nicopap
Copy link
Contributor

nicopap commented Sep 10, 2023

So do we prefer the historical AliceBlue or the X11/W3C AliceBlue? The PR description doesn't motivate the change.

@mockersf
Copy link
Member

I would prefer to keep to the web colors. The color from this PR is US centric and not used anywhere else

@mockersf mockersf added the X-Controversial There is active debate or serious implications around merging this PR label Sep 10, 2023
@CleanCut
Copy link
Member Author

Huh, okay. I can't actually tell the difference between pure white and the alice blue white on my screen, but I had no idea the whitish color was some web color thing.

@CleanCut CleanCut closed this Sep 10, 2023
@CleanCut CleanCut deleted the alice-blue branch September 10, 2023 22:36
@cart
Copy link
Member

cart commented Sep 13, 2023

Quick @alice-i-cecile just give us a random hex code and we'll use that as ALICE_BLUE.

@alice-i-cecile
Copy link
Member

#54b6e3. Now it's canon!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Rendering Drawing game state to the screen C-Bug An unexpected or incorrect behavior M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide X-Controversial There is active debate or serious implications around merging this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants