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

Add support for CSS colors, HSL and HWB, PDFColor class, update manual #724

Closed
wants to merge 2 commits into from

Conversation

dmeranda
Copy link

This is my attempt to overhaul the color handling internals in Prawn. It lets you use nearly all the CSS syntax to represent colors. It also introduces a Prawn::PDFColor base class to store instances of specific colors; of which a subclass Prawn::CSSColor contains all the CSS logic.

I also updated the manual to include a new section on colors in the Basic Concepts chapter.

I think this is well documented and tested, though since its not a trivial change more eyes would be appreciated.

@packetmonkey
Copy link
Contributor

So I dropped everything and looked over this PR, loving the idea.

I personally would like to see the various classes defined in their own files to make things easier it find.

It looks like to me you are using PDFColor as an abstract class ( as commented ), I wonder if instead of returning values that aren't ment to really be used in the system, those methods should raise NotImplementedError instead. That's a higher level style decision @sandal should sign off on I imagine.

Also not sure about presenting CSS color as the 'standard' as inferred in manual/basic_concepts/css_colors.rb but for a new user that may make the learning curve easier, and it's not like we dropped support for the other forms.

👍 from me having not yet pulled the branch down and played with it myself.

@packetmonkey
Copy link
Contributor

Also are there specs in this PR for the new code or am I just not seeing them?

@practicingruby
Copy link
Member

@packetmonkey, @dmeranda: Right now I'm looking to reduce the amount of features we're directly supporting in Prawn itself, so I'd much rather see this supported as a third-party extension rather than consider a pull request.

Happy to consider a discussion around what extension points we'd need to add to the API to facilitate that!

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.

3 participants