-
-
Notifications
You must be signed in to change notification settings - Fork 519
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
Added hex color conversion #1432
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- The constructor only tests the happy path.
- I think
public static Color.FromHex(string hex)
would be a more desirable approach here from an API perspective. - All errors will need to be resolved.
I'd vote for a combination of |
We use "ToHex" so "FromHex" and "TryFromHex" is more consistent API wise |
It looks like |
Unfortunately, the build still breaks @jsheely - see the build log above, looks like simple char/string conversion error. Want to update and push again? |
Thank you @jsheely, if no one else gets to it first, I'll review (and hopefully merge) your PR this week. |
cd50c9d
to
7e313bd
Compare
Dear @patriksvensson, I have rebased this branch, implemented full unit test coverage for both happy and unhappy paths, and ensured the failure to parse an invalid colour raises a I think it's suitable for merging, please re-review when you can. |
Do we want to support the short-hands for hex values? (E.g. using |
You tell me @nils-a 😉, as someone who would like to merge this PR for me... I'm happy to implement the short color code if you'd like. This (seemingly trivial) PR has been hanging around for too long for my liking. |
Agreed. Let's use it as it is. @patriksvensson I feel your requested changes were addressed, right? |
I'm pretty sure you want to merge this PR @nils-a . Remember that really nice, warm fuzzy feeling from doing so? It's waiting. |
@jsheely your changes have been merged, thanks for your contribution 👍 |
Solves issue #1354
I'm not sure if this is desired but seemed like an easy quality of life feature to allow hex to rgb for creating colors.
Please upvote 👍 this pull request if you are interested in it.