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

Full feature compatibility with WPF's Colors class #393

Merged
merged 10 commits into from Oct 11, 2016

Conversation

Projects
None yet
4 participants
@adrianknight89
Contributor

adrianknight89 commented Sep 29, 2016

Description of Change

WPF's Colors class has 141 colors from UNIX X11 color table. See https://msdn.microsoft.com/en-us/library/system.windows.media.colors(v=vs.110).aspx

XF had 19 colors, so I added the remaining for full compatibility.

Questions

  • XF's definition of Pink is wrong. It should be rgb(255, 192, 203). If we want to change it, it'll be a breaking change. How should this be handled? (Also XF Transparent is black with 0 alpha whereas .NET counterpart is white with 0 alpha. At least this doesn't represent a bug like that of Pink.)
  • I added hex values of the colors since msdn shows hex in its own documentation as well as the color table. Should the original 19 colors also show hex? See Color.cs
  • Original documentation is wrong in that it's using the wrong word RGB instead of HEX. For example: White, the color that is represented by the RGB value #ffffff. Should I fix this?

P.S. I automated most of the work, so there shouldn't be an error with the hex values.

@dnfclas

This comment has been minimized.

Show comment
Hide comment
@dnfclas

dnfclas Sep 29, 2016

Hi @adrianknight89, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by .NET Foundation and real humans are currently evaluating your PR.

TTYL, DNFBOT;

dnfclas commented Sep 29, 2016

Hi @adrianknight89, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by .NET Foundation and real humans are currently evaluating your PR.

TTYL, DNFBOT;

@StephaneDelcroix
  • I think we can "break" tinder-like apps, and redefine Pink.
  • #ffffff is an RGB value, represented as an hex triplet (#rrggbb). that's all correct.
  • the docs should be fixed to include the new colors
Show outdated Hide outdated Xamarin.Forms.Core/Color.cs
@@ -371,6 +371,130 @@ public static Color FromHsla(double h, double s, double l, double a = 1d)
public static readonly Color White = FromRgb(255, 255, 255);
public static readonly Color Yellow = FromRgb(255, 255, 0);
// remaining colors in WPF's System.Windows.Media.Colors

This comment has been minimized.

@StephaneDelcroix

StephaneDelcroix Oct 5, 2016

Member

I'd document this as remaining colors from Unix x11 color table (https://en.wikipedia.org/wiki/X11_color_names)

@StephaneDelcroix

StephaneDelcroix Oct 5, 2016

Member

I'd document this as remaining colors from Unix x11 color table (https://en.wikipedia.org/wiki/X11_color_names)

This comment has been minimized.

@StephaneDelcroix

StephaneDelcroix Oct 5, 2016

Member

but actually, I do not care

@StephaneDelcroix

StephaneDelcroix Oct 5, 2016

Member

but actually, I do not care

Show outdated Hide outdated Xamarin.Forms.Core/Color.cs
@@ -371,6 +371,130 @@ public static Color FromHsla(double h, double s, double l, double a = 1d)
public static readonly Color White = FromRgb(255, 255, 255);
public static readonly Color Yellow = FromRgb(255, 255, 0);
// remaining colors in WPF's System.Windows.Media.Colors
public static readonly Color AliceBlue = FromHex("#FFF0F8FF");

This comment has been minimized.

@StephaneDelcroix

StephaneDelcroix Oct 5, 2016

Member

we use FromRgb() on purpose. FromHex() is doing string parsing, and parsing 141 strings at start time, on resource-scarce devices is something we want to avoid if we can.

@StephaneDelcroix

StephaneDelcroix Oct 5, 2016

Member

we use FromRgb() on purpose. FromHex() is doing string parsing, and parsing 141 strings at start time, on resource-scarce devices is something we want to avoid if we can.

This comment has been minimized.

@adrianknight89

adrianknight89 Oct 6, 2016

Contributor

Changed to FromRgb().

@adrianknight89

adrianknight89 Oct 6, 2016

Contributor

Changed to FromRgb().

Show outdated Hide outdated Xamarin.Forms.Core/Color.cs
public static readonly Color Violet = FromHex("#FFEE82EE");
public static readonly Color Wheat = FromHex("#FFF5DEB3");
public static readonly Color WhiteSmoke = FromHex("#FFF5F5F5");
public static readonly Color YellowGreen = FromHex("#FF9ACD32");

This comment has been minimized.

@StephaneDelcroix

StephaneDelcroix Oct 5, 2016

Member

Everything should be sorted, the new with the old ones.

@StephaneDelcroix

StephaneDelcroix Oct 5, 2016

Member

Everything should be sorted, the new with the old ones.

This comment has been minimized.

@adrianknight89

adrianknight89 Oct 6, 2016

Contributor

@StephaneDelcroix Should the docs be sorted as well?

@adrianknight89

adrianknight89 Oct 6, 2016

Contributor

@StephaneDelcroix Should the docs be sorted as well?

This comment has been minimized.

@StephaneDelcroix

StephaneDelcroix Oct 6, 2016

Member

I don't think so. That's the responsibility of the displaying tool

@StephaneDelcroix

StephaneDelcroix Oct 6, 2016

Member

I don't think so. That's the responsibility of the displaying tool

This comment has been minimized.

@adrianknight89

adrianknight89 Oct 6, 2016

Contributor

Sorted colors alphabetically. Same was done in ColorTypeConverter.cs

@adrianknight89

adrianknight89 Oct 6, 2016

Contributor

Sorted colors alphabetically. Same was done in ColorTypeConverter.cs

@StephaneDelcroix

This comment has been minimized.

Show comment
Hide comment
@StephaneDelcroix

StephaneDelcroix Oct 5, 2016

Member

before fixing the docs and everything, wait for @jassmith opinion on whether or not we will include this.

Member

StephaneDelcroix commented Oct 5, 2016

before fixing the docs and everything, wait for @jassmith opinion on whether or not we will include this.

@adrianknight89

This comment has been minimized.

Show comment
Hide comment
@adrianknight89

adrianknight89 Oct 5, 2016

Contributor

I already provided the docs. GitHub won't show it because it's too big. You should be able to see it in your IDE.

Contributor

adrianknight89 commented Oct 5, 2016

I already provided the docs. GitHub won't show it because it's too big. You should be able to see it in your IDE.

@StephaneDelcroix

This comment has been minimized.

Show comment
Hide comment
@StephaneDelcroix

StephaneDelcroix Oct 5, 2016

Member

my wrong, they are indeed at the bottom of the changes. 👍

Member

StephaneDelcroix commented Oct 5, 2016

my wrong, they are indeed at the bottom of the changes. 👍

@StephaneDelcroix

This comment has been minimized.

Show comment
Hide comment
@StephaneDelcroix

StephaneDelcroix Oct 5, 2016

Member

@adrianknight89 we'll take this change in, including the change in the Pink Color. Could you update the PR ?

thanks

Member

StephaneDelcroix commented Oct 5, 2016

@adrianknight89 we'll take this change in, including the change in the Pink Color. Could you update the PR ?

thanks

@adrianknight89

This comment has been minimized.

Show comment
Hide comment
@adrianknight89

adrianknight89 Oct 6, 2016

Contributor

I updated Pink to reflect the correct value. I also updated Transparent to be transparent from white instead of black. The end result should be the same since the alpha is 0.

Should we remove the obsolete color? It was set to obsolete in 1.3. I think it's time to get rid of it and let devs update to the correct spelling if they haven't already.

Contributor

adrianknight89 commented Oct 6, 2016

I updated Pink to reflect the correct value. I also updated Transparent to be transparent from white instead of black. The end result should be the same since the alpha is 0.

Should we remove the obsolete color? It was set to obsolete in 1.3. I think it's time to get rid of it and let devs update to the correct spelling if they haven't already.

@StephaneDelcroix

This comment has been minimized.

Show comment
Hide comment
@StephaneDelcroix

StephaneDelcroix Oct 11, 2016

Member

@adrianknight89 unfortunately we can't remove API, even the ones Obsoleted for a long time.

Member

StephaneDelcroix commented Oct 11, 2016

@adrianknight89 unfortunately we can't remove API, even the ones Obsoleted for a long time.

@StephaneDelcroix StephaneDelcroix self-assigned this Oct 11, 2016

@StephaneDelcroix StephaneDelcroix merged commit 44397cb into xamarin:master Oct 11, 2016

@samhouts samhouts added this to the 2.3.4 milestone Jun 27, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment