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

Support custom colors #200

Merged
merged 1 commit into from
Apr 26, 2024
Merged

Conversation

peacog
Copy link

@peacog peacog commented Apr 24, 2024

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.

Copy link
Collaborator

@symbioquine symbioquine 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 working on this @peacog! This is a good idea!

src/styles/index.js Outdated Show resolved Hide resolved
README.md Outdated
Comment on lines 288 to 289
Custom colors in hex or rgb can be used by providing a `color` option with the
desired color value. rgb, rgba, and hex values with or without alpha channel are all supported.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Once that custom parsing code is removed, this can provide a few examples, then just point at the color-parse docs.

@mstenta
Copy link
Member

mstenta commented Apr 24, 2024

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

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

src/styles/index.js Outdated Show resolved Hide resolved
@peacog peacog force-pushed the 2.x branch 2 times, most recently from bc47eab to eab2e89 Compare April 25, 2024 07:23
@peacog
Copy link
Author

peacog commented Apr 25, 2024

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
Copy link
Collaborator

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
Copy link
Member

paul121 commented Apr 25, 2024

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
Copy link
Author

peacog commented Apr 26, 2024

I've updated the changelog and squashed the commits.

@symbioquine
Copy link
Collaborator

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...

@symbioquine symbioquine merged commit 6994de2 into farmOS:2.x Apr 26, 2024
2 checks passed
@mstenta
Copy link
Member

mstenta commented Apr 26, 2024

Thanks again @peacog!!! 🎉

@symbioquine
Copy link
Collaborator

symbioquine commented Apr 26, 2024

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: farmOS/farmOS@42fe4bc

@peacog
Copy link
Author

peacog commented Apr 26, 2024

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

@peacog
Copy link
Author

peacog commented May 2, 2024

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

@symbioquine
Copy link
Collaborator

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
Copy link
Author

peacog commented May 2, 2024

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

@symbioquine
Copy link
Collaborator

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: farmOS/farmOS#841

@peacog
Copy link
Author

peacog commented May 8, 2024

thanks @symbioquine !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants