farmOS / farmOS-map

farmOS-map is an OpenLayers wrapper library designed for agricultural mapping needs. It can be used in any project that has similar requirements.
https://farmOS.github.io/farmOS-map
MIT License
40 stars 22 forks source link

Support custom colors #200

Closed peacog closed 6 months ago

peacog commented 6 months ago

If we want to use stroke colors other than the 8 predefined named colors it's necessary to write a style function. It would be great if we could simply pass the hex or rgb value in the color option. This PR does that. The color option can be one of the predefined color names, a hex value with or without alpha channel, or an rgb() or rgba() value.

mstenta commented 6 months ago

This is great @peacog! Thanks for the contribution! +1

And cool to learn that OpenLayers can handle some of that parsing logic too @symbioquine. :-)

peacog commented 6 months ago

Thanks very much @symbioquine! Your suggestion is a much better solution and easier to understand. I had to assign to a new variable to avoid a Assignment to function parameter lint error, and I've also updated the readme :)

symbioquine commented 6 months ago

I think we're pretty close to being able to merge this. @peacog Can you please add your change to the CHANGELOG and squash your changes.

paul121 commented 6 months ago

Can you please add your change to the CHANGELOG and squash your changes.

How would we feel about doing a squash and merge here? I know we usually try and keep clean commit histories but especially for something like this when it is a simple change doesn't need multiple commits to explain the changes there's something nice about preserving the history from this PR here and squashing into a single commit on merge. I hope this could somewhat lower the barrier to contributions/maintenance, too

peacog commented 6 months ago

I've updated the changelog and squashed the commits.

symbioquine commented 6 months ago

Can you please add your change to the CHANGELOG and squash your changes.

How would we feel about doing a squash and merge here? I know we usually try and keep clean commit histories but especially for something like this when it is a simple change doesn't need multiple commits to explain the changes there's something nice about preserving the history from this PR here and squashing into a single commit on merge. I hope this could somewhat lower the barrier to contributions/maintenance, too

@paul121 I feel fine about using the squash and merge functionality, but in this case I could see there wasn't a changelog entry so somebody is going to have to author 1 more commit adding that. At the point that's happening, it seems better for the original requester to do the work of consolidating the commit messages and getting the change ready-to-merge.

I could also have just hit "approve" and let the workflow for the changelog check fail, but that feels less personal - almost passive aggressive compared to asking the PR author for what needs to happen...

mstenta commented 6 months ago

Thanks again @peacog!!! :tada:

symbioquine commented 6 months ago

Thanks for the contribution to farmOS-map @peacog! If you need this functionality in farmOS, the next step is a release of farmOS-map and a change like this one bumping the version of farmOS-map that ships with farmOS: https://github.com/farmOS/farmOS/commit/42fe4bc50eb499d30628b8d2722877accd1e25a7

peacog commented 6 months ago

Great! thanks very much to everyone for helping with this and accepting it so quickly :)

peacog commented 6 months ago

Hi @symbioquine. Is created a new release something I can do? What are the steps?

symbioquine commented 6 months ago

There's not much to it. I think the next version should be something like 2.3.1.

There needs to be a commit like this one and a corresponding tag (https://github.com/farmOS/farmOS-map/releases/tag/v2.2.2) pushed. In theory you could create another PR for the commit, but @paul121, @mstenta, or myself need to push the tag so it's probably easier for one of us to do both the commit and the tagging.

I can do it by Tuesday morning at the latest - possibly sooner, but no promises since I'm going to be traveling...

peacog commented 6 months ago

I'm in no rush @symbioquine Whenever you find time to do it is fine by me. Thanks!

symbioquine commented 6 months ago

I made the v2.3.0 release of farmOS-map that includes this change and opened a pull request to bump the version used by farmOS: https://github.com/farmOS/farmOS/pull/841

peacog commented 6 months ago

thanks @symbioquine !