vaab / colour

Python color representations manipulation library (RGB, HSL, web, ...)

BSD 2-Clause "Simplified" License

319 stars 41 forks source link

Adding the XKCD colour definition to the library #55

Open Gnu-Bricoleur opened 3 years ago

Gnu-Bricoleur commented 3 years ago

Hi, I've been using your module for some time and I was thinking the XKCD colour definition would be a fine addition to it. https://blog.xkcd.com/2010/05/03/color-survey-results/ This might seem a little bit odd at first as it's not really an accepted standard anywhere but the methodology behind this way of defining colours name is sound and at least one other python library is implementing them (matplotlib). Also, it's fun ! Let me know what you think of it. I can complete the documentation if you like this pull request. Also, it might be cleaner to define standard RGB colours in a separate file too so the the architecture is the same for both dictionaries defining colours name but i didn't want to modify too much the library for this first pull request.

vaab commented 3 years ago

Thanks, that was a good read. I'm totally open to integrate xkcd naming into colour. Although, the current proposal in itself could use a little more polishing:

First, it is far from even working with the current posted code in your current PR, for numerous reasons
Then, if I like some of your choices, some of them, I'll need to think them through, for instance
- using uppercase in identifier for xkcd,
- how to include them : I can't have them pop out in the 'web' color names as these are used to produce names in CSS for instance. Not sure that firefox / Chrome are ready for all these identifiers.
For me, it is a new color naming domain as 'web', 'rgb', 'hsl' ... , 'xkcd' would be a new one.

I would understand that you don't want to push further this PR yourself, and if you don't mind waiting a little, I'll surely integrate this myself, but with a low priority on my list.

After said that, you are welcome to make a better PR if you feel so. I'll send a few remark on your code to give you more clues about what to improve.

Gnu-Bricoleur commented 3 years ago

Whooo, first of all, thanks a lot for taking the time to review my request I really appreciate your feedback and I'm going to take the opportunity and try to make a good pull request ! I'm going to look through your comments and correct everything

vaab commented 3 years ago

Don't hesitate to push unfinished code in this PR to ask for confirmation on some of your choices, this could help you not loosing to much time on things I might refuse. You may probably also want to start by the documentation so as to make sure that we have the same view on what is the objective for the final user.

codecov[bot] commented 3 years ago

Codecov Report

:exclamation: No coverage uploaded for pull request base (master@11f138e). Click here to learn what that means. The diff coverage is 75.00%.

@@            Coverage Diff            @@
##             master      #55   +/-   ##
=========================================
  Coverage          ?   98.72%           
=========================================
  Files             ?        1           
  Lines             ?      236           
  Branches          ?        0           
=========================================
  Hits              ?      233           
  Misses            ?        3           
  Partials          ?        0

Impacted Files	Coverage Δ
colour.py	`98.72% <75.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 11f138e...635483e. Read the comment docs.

Gnu-Bricoleur commented 3 years ago

Hi Valentin,

So I tried to take into account your remarks and include the XKCD naming scheme in a similar fashion to the web color. It's still incomplete and it's not possible to get the xkcd name from a color defined with hex values. However, the basics works => defining a color with a xkcd code name and it's possible to use it as a normal color object afterwards. c = Color(xkcd="baby_blue") What do you think of it ? Is it going in the right direction according to you ?

Gnu-Bricoleur commented 3 years ago

Hi Valentin,

I just wanted to check with you if you had the time to get a look at the modifications I made to the request?

Regards, Sylvain