Skip to content

Add Blend Modes and Canonical Show instances for Enumerations #32

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Mar 11, 2016
Merged

Add Blend Modes and Canonical Show instances for Enumerations #32

merged 2 commits into from
Mar 11, 2016

Conversation

igagen
Copy link
Contributor

@igagen igagen commented Mar 10, 2016

Did you have something else in mind for the show implementation?

paf31
paf31 reviewed Mar 10, 2016
View reviewed changes
src/Graphics/Canvas.purs
@@ -482,6 +483,15 @@ foreign import drawImageFull :: forall eff. Context2D -> CanvasImageSource -> Nu
-- | Enumerates the different types of pattern repetitions.
data PatternRepeat = Repeat | RepeatX | RepeatY | NoRepeat

instance showPatternRepeat :: Show PatternRepeat where
show Repeat = "repeat"
Copy link
Contributor

Choose a reason for hiding this comment

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

What I was getting at before was that we expect valid Show instances to produce strings which are executable code. So this and the Show TextAlign instance should be changed.

I was suggesting the current show be turned into toString which would be put into a where clause somewhere and not exported. And ideally the same for TextAlign.

@igagen igagen force-pushed the patterns branch 4 times, most recently from ea4dcf1 to 3f9f7b5 Compare March 11, 2016 02:36
@igagen igagen changed the title Add Show instance for PatternRepeat, as well as toString helper Add Show instances for PatternRepeat, TextAlign, Composite Mar 11, 2016
@igagen
Copy link
Contributor Author

igagen commented Mar 11, 2016

Ok, I updated the Show instances for TextAlign and PatternRepeat, and Composite as well, and replaced the show calls with toString as a private helper in the where clause. By the way, I noticed that a bunch of the newer composite operations are not in Composite (e.g. multiply, screen, etc.). I could add those in if you want.

@paf31
Copy link
Contributor

paf31 commented Mar 11, 2016

This looks great, thanks!

I could add those in if you want.

That would be good, if you don't mind.

@igagen
Copy link
Contributor Author

igagen commented Mar 11, 2016

Ok great, I'll add those as well :)

@igagen igagen changed the title Add Show instances for PatternRepeat, TextAlign, Composite Add Blend Modes and Canonical Show instances for Enumerations Mar 11, 2016
@igagen
Copy link
Contributor Author

igagen commented Mar 11, 2016

Just added the new blend modes (https://www.w3.org/TR/compositing-1/).
The changes overlapped the Show/toString stuff, so I did it on this PR rather than opening another.

@paf31
Copy link
Contributor

paf31 commented Mar 11, 2016

Fantastic, thank you!

paf31 added a commit that referenced this pull request Mar 11, 2016
@paf31
Merge pull request #32 from igagen/patterns
f8d1fa2 
Add Blend Modes and Canonical Show instances for Enumerations
@paf31 paf31 merged commit f8d1fa2 into purescript-web:master Mar 11, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@igagen @paf31