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

Implements Rgba::from_str() #347

Merged
merged 5 commits into from
Aug 25, 2023

Conversation

rafaelbeckel
Copy link
Contributor

This change implements the from_str() trait for the Rgba type, adding support for parsing a RGBA hex string with the format #rrggbbaa.

No related issue, no breaking changes.

Copy link
Owner

@Ogeon Ogeon left a comment

Choose a reason for hiding this comment

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

Thanks for taking the time to add this! I had a comment (2 about the same thing) regarding the error, but the rest looks good.

I'll take a proper look on Sunday or Monday, when I have proper computer access again, but I'm sure it will be ready to be merged as soon as the error comment(s) can be addressed. Feel free to remind me if I seem to have forgotten. 😅

palette/src/rgb/rgb.rs Outdated Show resolved Hide resolved
palette/src/rgb/rgb.rs Outdated Show resolved Hide resolved
@Ogeon
Copy link
Owner

Ogeon commented Aug 18, 2023

Another thing to consider is to have an entirely different error type for rgba. I'm not sure how helpful it would be.

@Ogeon
Copy link
Owner

Ogeon commented Aug 21, 2023

The tests should be fixed with #349, so you will have to rebase (or merge) this PR to get those changes here as well.

@Ogeon
Copy link
Owner

Ogeon commented Aug 21, 2023

Anyway, I'm back from my adventures (some nice mountain hiking) and gave this another look. Apart from the error handling, I would suggest adding support for the 4 character shorthand format, too. It may not be as common, but I think it makes sense to at least be consistent with Rgb<S, u8> in that way. But that would be all!

@rafaelbeckel
Copy link
Contributor Author

Another thing to consider is to have an entirely different error type for rgba. I'm not sure how helpful it would be.

I tried that, but it turned out to be too verbose, and the definitions became too far away from the usage. I decided to stick with a variant instead. It makes more sense semantically, too.

Copy link
Owner

@Ogeon Ogeon left a comment

Choose a reason for hiding this comment

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

Thanks for checking the separate error idea. I agree that it sounds better to have only one type.

palette/src/rgb/rgb.rs Outdated Show resolved Hide resolved
@Ogeon
Copy link
Owner

Ogeon commented Aug 25, 2023

The code looks good and the tests are all green. 🙂 Thanks a lot!

@Ogeon
Copy link
Owner

Ogeon commented Aug 25, 2023

bors r+

@bors
Copy link
Contributor

bors bot commented Aug 25, 2023

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

@bors bors bot merged commit 9ce4a63 into Ogeon:master Aug 25, 2023
15 checks passed
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.

2 participants