feat: external from_hex function#212
Conversation
|
This is handled by |
|
Yes, that's correct. |
There was a problem hiding this comment.
Welcome! I really appreciate the spirit of coming to improve infrastructure directly, and wish more contributors would do so!
Unfortunately, we can't land this PR as-is, due to the numerous issues with boundary conditions here.
I have a few thoughts:
- We should probably make it easier to transition a
DynamicColorto anAlphaColor(edit: This isDynamicColor::to_alpha_color) - I believe that an sRGB-only
from_hexfunction could be valuable, if (and possibly iff) we can make it const, and the input string gets a colour picker in vscode. - Our documentation about how to create a colour is lacking. I believe the correct place for that would actually be in Peniko, although adding doc examples to the colour types wouldn't be amiss.
Practically, the way forward I'd suggest for this PR is either of the following:
- add a
const fn from_hexwhich does the right thing, iff our MSRV permits it. The reason for that function existing would be being const. - adding an example to any non-empty subset of
DynamicColor,AlphaColor,OpaqueColorandPremulColor. For AlphaColor, we should also mention thecssmodule, and for DynamicColor we should mention theFromimplementation.
If you want to do both, the latter would ideally be a separate PR to make review easier.
|
I'd be more inclined to say we should impl |
|
Oh! We already do. I guess this really is a documentation thing then. |
|
It is not possible to produce a |
|
I think that a A function called I do think that improved documentation here would still be valuable. |
|
I've used |
DJMcNab
left a comment
There was a problem hiding this comment.
Thank you for taking on the review feedback. This is so much better ❤️
I probably won't get to this until next week. A couple of thoughts:
- This should handle a leading
#. I think having it be optional would be fine. - I would lean towards giving a parse error if 4 or 8 channels are specified in
OpaqueColor::from_hex(i.e. an alpha was provided for an opaque colour. - we should have a handful of simple tests of this functionality.
- we should validate that this works In a const context, including unwrapping being ergonomic. If we can't unwrap (which I think we probably can't...), then I think it would be worth panicking on failure instead
- the docs should be expanded slightly.
Of these, 1 and 3 would block this landing. 2 is a matter of taste, and I'm happy for this to land without it. 4 is important, but could be deferred, so long as we were careful to resolve it before a release. I'm happy to complete 5 myself just before merging.
| /// Create a color from a hexadecimal value. | ||
| pub const fn from_hex(hex: &str) -> Result<Self, ParseError> { |
There was a problem hiding this comment.
The documentation should at the very least:
- Point to
parse_colorand explain the differences with it. - Include one doctest showing how to use the function.
This PR adds a new Color function
from_hexin addition tofrom_rgb8andfrom_rgba8.I'm not sure about the name, since it does more than just hex, so maybe
from_css?