Skip to content

Conversation

@AmjadHD
Copy link
Contributor

@AmjadHD AmjadHD commented Jul 13, 2022

image.fill("white") instead of image.fill(parseHtmlColor "white")

`image.fill("white")` or `colorStop("#00F", 2)` just works
@guzba
Copy link
Collaborator

guzba commented Jul 13, 2022

Hello and thanks for the suggestion. I do not support having an invisible automatic converter for all strings whenever pixie is imported. This is simple enough to have in your own package if you are working with strings.

@AmjadHD
Copy link
Contributor Author

AmjadHD commented Jul 13, 2022

I see your point, but I don't see the harm here, it only converts a string when a color is required which is convenient.

@guzba
Copy link
Collaborator

guzba commented Jul 13, 2022

This issue is related to the fact that fill specifically takes only SomeColor, and not a Paint. We already have a converter to Paint https://github.com/treeform/pixie/blob/master/src/pixie/paints.nim#L48 Adding this would be multiple converters on string that also could convert in between (string -> Color -> Paint).

We have talked about converting fill to taking a Paint, or having an overload that does, however it is tied to other things (what is a fill of an ImagePaint?). I think we'll end up having a fill with a Paint though eventually, which will enable this usage of string. We'll just treat fill as the equivalent of drawing a rect over the entire image with the Paint as paths.nim already does.

Once we have that, this converter will not be needed, so I see this as a dead end and not something I want to support long term nor break for anyone who started using it.

@guzba
Copy link
Collaborator

guzba commented Jul 13, 2022

I will use this issue as a reminder to prototype the fill taking Paint conversion. I have some travel coming up this week but after that I hope to see if it turns out to be no big deal. Makes the API more consistent and resolves this string convenience issue.

@guzba
Copy link
Collaborator

guzba commented Jul 24, 2022

Put a PR together for the image.fill(paint) idea, which enables using strings for fill without needing to add another converter type. See https://github.com/treeform/pixie/pull/473/files#diff-4e191d354bc826b6cfb448ef125b2055bb7d482a702e4010ac0e205864e64252R258 for a test, pr is #473

@guzba
Copy link
Collaborator

guzba commented Jul 24, 2022

Hello again. Thanks for opening this. We have now merged an alternative approach to enable using fill with a string for color: #473

This has not been tagged in a release yet but will be included when we tag our next release. In the meantime you can try it out if you use #head of Pixie.

@guzba guzba closed this Jul 24, 2022
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