Grid Support #210

CharlesTaylor7 · 2020-07-24T21:55:39Z

Resolves #176

Explicit support for all grid properties listed on mdn, except for a handful of shorthand properties:

The above shorthands have involved syntax which would make them a pain to support. The finer grained methods still encapsulate all the grid layout options.

I'm mostly happy with how this pr turned out. The main shortcoming for me is upcast. I tried a number of other things, but none of them worked very well. What would be nice here is Impredicative Polymorphism. I want to be able to create a list of type [forall a. Size a] for gridTemplateRows/gridTemplateColumns, but GHC won't oblige me. Maybe something to revisit when we get Quick Look?

turion · 2020-07-26T15:08:41Z

Exciting :) do assign me as a reviewer when you're ready.

CharlesTaylor7 · 2020-07-26T19:03:07Z

@turion Ready for review. I don't think have permissions for explicitly assigning you as reviewer.

turion

Thanks a lot for the extensive work so far! Here are some initial comments to get the review started. I'm going to have a second round later, when I understand your angle better.

turion · 2020-08-01T11:48:18Z

clay.cabal

@@ -75,7 +75,8 @@ Library
  Build-Depends:
    base  >= 4.11 && < 5,
    mtl   >= 1    && < 2.3,
-    text  >= 0.11 && < 1.3
+    text  >= 0.11 && < 1.3,
+    these < 1.2


If you introduce an upper bound, shouldn't you also introduce a lower bound?

I looked at the earliest version of the package on hackage, and it has the constructurs I'm making use of.
But I can add a lower bound for completeness.

Yes that would be nice, then noone is going to ask themselves that question anymore.

turion · 2020-08-01T11:58:52Z

src/Clay/Size.hs

+
+-- | Upcast a size unit
+upcast :: Size a -> Size AnyUnit
+upcast = coerce


Sorry, I think this is not a good idea. Can't you instead:

Make Size into a GADT

Add an Upcast :: Size a -> Size AnyUnit constructor

Can you elaborate?
What's the functional difference from the user perspective?
It's more work to embed it in a constructor, I now have to add cases to sizeToText & the Val instance that do nothing but unwrap.

From the user perspective it's mostly the same I hope. But from the maintainer's perspective a huge burden is gone because noone has to worry about coerce breaking anything.

I have to admit that I completely misunderstood what you did here. I mistook coerce for unsafeCoerce, which is - as the name says - unsafe, whereas coerce is completely fine. It will never break anything because the type checker ensures that coercion only happens when the internal representations are indeed the same. If that was your goal, I'm sorry about the misdirection: feel free to disregard the stuff I said about GADTs, and let's make everyone's life easier by using coerce.

turion · 2020-08-01T12:04:24Z

spec/Clay/SizeSpec.hs

@@ -47,8 +42,6 @@ spec = do
      sizeRepr (em 2 @+@ px 1) `shouldBe` "calc(2em + 1px)"
    it "returns calc for nested sum" $
      sizeRepr (em 2 @+@ pt 1 @+@ px 3) `shouldBe` "calc((2em + 1pt) + 3px)"
-    it "returns prefixed calc for simple sum" $


Why is this removed?

turion · 2020-08-01T12:05:43Z

spec/Common.hs

@@ -1,8 +1,16 @@
-module Common where
+module Common
+  ( shouldRenderFrom


I think there is no need to filter the exports. Or was there a name clash?

No name clash. I introduced a new top level binding of testRender & was preventing its export. It's not a big deal either way.

Ah, I'm fine if you don't restrict the exports in that case.

src/Clay/Grid.hs

CharlesTaylor7 · 2020-08-01T14:06:34Z

@turion To be clear, you're very opposed to partial Num instances which exist just for number literals?

But are you open to partial IsList instances or no?
I will point out that NonEmpty has a partial fromList implementation for its IsList instance. I find it useful, but I recognize that its debatable and a matter of taste.

turion · 2020-08-03T08:56:27Z

@turion To be clear, you're very opposed to partial Num instances which exist just for number literals?

But are you open to partial IsList instances or no?
I will point out that NonEmpty has a partial fromList implementation for its IsList instance. I find it useful, but I recognize that its debatable and a matter of taste.

In my own libraries, I would not allow any partial functions that are reachable for library users. In clay, which I didn't author and only maintain so it is preserved for the community, there already are a lot of partial functions. They partly exist in places where I don't see their justification: For example Size Percentage has a Num instance without a (+) function, but with a refactoring of the code it would be possible to define (+) and all the other operations. Given enough time and dev effort, we can of course remove all of these warts, but time and effort are limited resources.

In particular for a CSS processor many people will take the stance that in a sense clay is always run "before runtime" in that CSS is generated before the webserver starts. Still these errors don't show up during type checking and are therefore not in the typical Haskell dev workflow. You'd think that your program will run when it compiles, but it doesn't necessarily here.

Since I don't want to undermine your efforts, I'd be willing to accept these partial functions if you really think they add value and cannot made be total with reasonable effort.

But if it's just for being able to write an integer literal I'm not so sure it's all that much added value. Consider shadow 23 42 vs shadow (px 23) (pct 42). The first expression is weirdly polymorphic in the size type in that you can later decide whether your 23 is percentage or pixels. But the idiomatic style is the second one anyways, which specifies the length units and types. It's clearer and not very long. The Num instance wasn't even used. I'd expect it to be similar in your applications, but of course you will be able to judge this better.

ddssff · 2021-06-12T20:08:52Z

Would love to see this merged.

turion · 2021-06-15T15:51:30Z

Yes, that would be great :)

@CharlesTaylor7 do you want to continue? I hope my review so far wasn't to rigorous to demotivate. Reading back what I wrote it sounds quite harsh in my ears. If it did that for you as well, I'm really sorry, and I'll try to write in a friendlier tone :)

If you have any questions on how to proceed, feel free to ask.

CharlesTaylor7 · 2022-01-08T16:13:27Z

Yes, that would be great :)

@CharlesTaylor7 do you want to continue? I hope my review so far wasn't to rigorous to demotivate. Reading back what I wrote it sounds quite harsh in my ears. If it did that for you as well, I'm really sorry, and I'll try to write in a friendlier tone :)

If you have any questions on how to proceed, feel free to ask.

@turion I would like to complete this PR. I hate loose ends and do appreciate quality focused, thorough code reviews. I have no issue with you, just lost interest with this last year. I have the time and motivation to tackle this now.

CharlesTaylor7 · 2022-01-08T19:03:59Z

@turion #210 (comment)
Do you have a recommended naming scheme for handling complex data definitions?
I think it's a benefit, not a disadvantage, for the record type name, to have a similar name to the sum type constructor's name, this links the two clearly. Though I do agree there needs to be a convention to know which is which.

For sum types, I took the approach of prefixing constructor names with the name of type. This can be done with or without underscores. I removed the underscores, because they are at odds with the rest of the library which doesn't use underscores. (Though, I do think the underscores make it easier to visually parse).

turion · 2022-01-09T11:27:52Z

src/Clay/Grid.hs

+-- https://developer.mozilla.org/en-US/docs/Web/CSS/grid-column-start
+-- either grid index and/or named grid area is required, but span is optional
+-- Note: constuctor is not exported, 'GridIndex, or 'gridLocationData to fill this info in
+data GridLocationData = MkGridLocationData IsSpan (These Integer GridArea)


The Constructor should have the same name as the type.

src/Clay/Grid.hs

turion · 2022-01-09T11:47:54Z

I would like to complete this PR.

Fantastic, looking forward to reviewing further :)

@turion #210 (comment) Do you have a recommended naming scheme for handling complex data definitions? I think it's a benefit, not a disadvantage, for the record type name, to have a similar name to the sum type constructor's name, this links the two clearly. Though I do agree there needs to be a convention to know which is which.

For sum types, I took the approach of prefixing constructor names with the name of type. This can be done with or without underscores. I removed the underscores, because they are at odds with the rest of the library which doesn't use underscores. (Though, I do think the underscores make it easier to visually parse).

This is a tough problem that I also often have. A few rules of thumb that I have for these situations:

Newtypes & product types have to have the same constructor name as the type name (GridLocationData in this example)
There cannot be a constructor with the name of an existing, distinct type (because otherwise people will think that the previous rule applies)
In sum types, ideally the constructor name doesn't need to repeat the type name. If collisions occur, avoid them by first thinking about different naming, and then putting types into submodules.

Specifically for clay, I'd add another rule of thumb:

If some Haskell definition matches exactly some known CSS concept, the naming should exactly mirror the CSS naming (adapted to camelCase and UpperCamelCase).

In your case though, I'd simply remove the GridLocationData type and add the fields to the corresponding constructor of the GridLocation type, though. The GridLocationData type isn't used anywhere else, and the total size of instance code will not be higher I think.

turion · 2022-01-09T11:48:44Z

src/Clay/Grid.hs

+data IsSpan = Span | NoSpan
+  deriving (Show, Eq)
+
+data GridLocation


Since this is not a known CSS concept (as far as I know), describe a bit what it is and what it is good for.

turion · 2022-01-09T11:49:24Z

src/Clay/Grid.hs

+
+-------------------------------------------------------------------------------
+
+gridRowStart :: GridLocation -> Css


But I thought grid rows don't support spans? See https://www.w3schools.com/cssref/pr_grid-row-start.asp. Maybe GridLocation should be GridColumnLocation?

turion · 2022-01-09T11:52:56Z

src/Clay/Common.hs

+class Hidden     a where hidden     :: a
+class Initial    a where initial    :: a
+class Unset      a where unset      :: a
+class MinContent a where minContent :: a


Is there any chance that some other types than Value will also be instances of your new classes?

turion · 2022-01-09T11:55:16Z

src/Clay/Grid.hs

-, gridTemplateColumns
-)
-where
+  ( gap


Are there many definitions in this module that are not exported? If everything is exported, we can also simply not restrict the exports.

turion · 2022-01-09T11:55:56Z

src/Clay/Grid.hs


 -- | Property sets the gaps (gutters) between rows and columns.
-gridGap :: Size a -> Css
-gridGap = key "grid-gap"
+-- Sets both "gap" & "grid-gap"


Suggested change

-- Sets both "gap" & "grid-gap"

--

-- Sets both "gap" & "grid-gap"

turion · 2022-01-09T11:56:30Z

src/Clay/Grid.hs

+gap = key "gap" <> key "grid-gap"
+
+-- | Property sets the size of the gap (gutter) between an element's grid rows.
+-- Sets both "row-gap" & "grid-row-gap"


Suggested change

-- Sets both "row-gap" & "grid-row-gap"

--

-- Sets both "row-gap" & "grid-row-gap"

At least I believe this will make a paragraph in haddock.

turion · 2022-01-09T11:57:25Z

src/Clay/Grid.hs

+rowGap = key "row-gap" <> key "grid-row-gap"
+
+-- | Property sets the size of the gap (gutter) between an element's grid columns.
+-- Sets both "column-gap" & "grid-column-gap"


turion · 2022-01-09T11:59:19Z

src/Clay/Grid.hs

+gridTemplateColumns = key "grid-template-columns"
+
+newtype GridTrackList a = GridTrackList Value
+  deriving (Val, None, Inherit, Initial, Unset)


Suggested change

deriving (Val, None, Inherit, Initial, Unset)

deriving (Val, None, Inherit, Initial, Unset, Auto, MaxContent, MinContent)

turion · 2022-01-09T12:01:40Z

src/Clay/Grid.hs

+gridTemplateColumns :: GridTrackList a -> Css
+gridTemplateColumns = key "grid-template-columns"
+
+newtype GridTrackList a = GridTrackList Value


Since Size is also an instance of None, Initial, etc., why not define it directly as a list of sizes?

Suggested change

newtype GridTrackList a = GridTrackList Value

newtype GridTrackList a = GridTrackList [Size a]

turion · 2022-01-09T12:06:55Z

src/Clay/Grid.hs

+gridTemplateColumns :: GridTrackList a -> Css
+gridTemplateColumns = key "grid-template-columns"
+
+newtype GridTrackList a = GridTrackList Value


My understanding now is that GridTrackList is a concept that has no corresponding CSS name, and it mirrors the possible values one can give to grid-template-columns and grid-template-rows. Maybe the naming can reflect this, and stay closer to known concepts? So for example GridTemplateSizes?

Or maybe these are called "tracks" in the community, and I just don't know?

turion · 2022-01-09T12:07:24Z

src/Clay/Grid.hs

+mkGridTrackList :: [Size a] -> GridTrackList a
+mkGridTrackList = GridTrackList . noCommas


Suggested change

mkGridTrackList :: [Size a] -> GridTrackList a

mkGridTrackList = GridTrackList . noCommas

gridTrackList :: [Size a] -> GridTrackList a

gridTrackList = GridTrackList . noCommas

But see https://github.com/sebastiaanvisser/clay/pull/210/files#r780772464 first.

src/Clay/Grid.hs

turion · 2022-01-09T12:08:21Z

src/Clay/Grid.hs

+gridAutoColumns :: GridAutoTrackList a -> Css
+gridAutoColumns = key "grid-auto-columns"
+
+newtype GridAutoTrackList a = GridAutoTrackList Value


The same comments as for GridTrackList apply.

turion

First superficial round done. I like where this is going!

Co-authored-by: Manuel Bärenz <[email protected]>

nsaunders · 2022-09-16T23:06:02Z

spec/Clay/GridSpec.hs

+        gridTemplateColumns none
+
+    describe "list of sizes" $ do
+      "{grid-template-columns:1em calc(20% + 1fr) auto}"


From the grid spec:

Note: <flex> values are not <length>s (nor are they compatible with <length>s, like some <percentage> values), so they cannot be represented in or combined with other unit types in calc() expressions.

CharlesTaylor7 · 2023-07-20T21:49:37Z

So I obviously lost steam to work on this, and there's a newer pull request in place, so withdrawing this PR. #250

CharlesTaylor7 changed the title ~~Grid support~~ Grid Support Jul 24, 2020

CharlesTaylor7 marked this pull request as ready for review July 26, 2020 19:01

turion requested changes Aug 1, 2020

View reviewed changes

CharlesTaylor7 added 20 commits January 8, 2022 11:26

Implement more of the Css grid spec

10ad518

Set linux style line endings

ec6e6a2

Tentative changelog

cbac4f5

Wrap each row in quotes

d70aa02

Restore gridGap, but deprecate it

4da0bd8

Test gridTemplateAreas

56b89a8

Remove deepseq dependency

414ea66

Formatting

b0dd86b

Use smart constructor pattern

298835a

Allow keyword values for gridTemplateAreas

f8c3e2f

Test stuff

7ecce2d

Test gridArea

afb5b6c

Mixed units in gridTemplateRows/gridTemplateColumns

21cf7a6

Integer & keyword grid coordinates

6ff4864

Use Data.These to model GridLocation

c9d875a

Fix a test

1265b88

Use bi-directional pattern to simplify

b381784

Drop browser prefixes from calc

21c7d41

update statistic

367fc84

Implement minmax css function

7c0568d

Handle underscores

874a1f5

CharlesTaylor7 added 4 commits January 8, 2022 14:12

Rename type

561cdae

Replace partial instance with a constructor

da09a3e

Remove more partial instances

53a5bb8

Don't derive exception

6cf06cd

turion reviewed Jan 9, 2022

View reviewed changes

src/Clay/Grid.hs Outdated Show resolved Hide resolved

turion reviewed Jan 9, 2022

View reviewed changes

src/Clay/Grid.hs Outdated Show resolved Hide resolved

turion reviewed Jan 9, 2022

View reviewed changes

turion requested changes Jan 9, 2022

View reviewed changes

CharlesTaylor7 and others added 2 commits January 9, 2022 16:59

Fix typo

5250c66

Co-authored-by: Manuel Bärenz <[email protected]>

Remove invalid keywords from GridAutoTrackList

148bf30

Co-authored-by: Manuel Bärenz <[email protected]>

nsaunders mentioned this pull request Mar 3, 2022

Support css grid properties purescript-contrib/purescript-css#112

Open

nsaunders reviewed Sep 16, 2022

View reviewed changes

nsaunders mentioned this pull request Oct 14, 2022

Grid nsaunders/purescript-tecton#3

Closed

CharlesTaylor7 closed this Jul 20, 2023


		-------------------------------------------------------------------------------

		gridRowStart :: GridLocation -> Css

	-- Sets both "gap" & "grid-gap"
	--
	-- Sets both "gap" & "grid-gap"

	-- Sets both "row-gap" & "grid-row-gap"
	--
	-- Sets both "row-gap" & "grid-row-gap"

	deriving (Val, None, Inherit, Initial, Unset)
	deriving (Val, None, Inherit, Initial, Unset, Auto, MaxContent, MinContent)

	newtype GridTrackList a = GridTrackList Value
	newtype GridTrackList a = GridTrackList [Size a]

		mkGridTrackList :: [Size a] -> GridTrackList a
		mkGridTrackList = GridTrackList . noCommas

Grid Support #210

Grid Support #210

Conversation

CharlesTaylor7 commented Jul 24, 2020 • edited Loading

turion commented Jul 26, 2020

CharlesTaylor7 commented Jul 26, 2020

turion left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CharlesTaylor7 commented Aug 1, 2020

turion commented Aug 3, 2020

ddssff commented Jun 12, 2021

turion commented Jun 15, 2021

CharlesTaylor7 commented Jan 8, 2022

CharlesTaylor7 commented Jan 8, 2022

Choose a reason for hiding this comment

turion commented Jan 9, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

turion Jan 9, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

turion left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CharlesTaylor7 commented Jul 20, 2023

CharlesTaylor7 commented Jul 24, 2020 •

edited

Loading

turion Jan 9, 2022 •

edited

Loading