-
Notifications
You must be signed in to change notification settings - Fork 193
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
Move Pagination to CSS framework #1157
Move Pagination to CSS framework #1157
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess at this point I have more questions than answers 😸
-- | Options for customizing the render of a pagination | ||
data PaginationView = | ||
PaginationView | ||
{ cssFramework :: !CSSFramework |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need a cssFramework
property if we also call styledPagination
with CSSFramework
? Seems that te pattern also with styledFormField
we later end up calling renderedHtml = styledPagination theCSSFramework theCSSFramework paginationView
. How many theCSSFramework
do we need? 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that's a bit tricky :) Basically we simulate late-binding from OOP here.
This code:
renderedHtml = styledPagination theCSSFramework theCSSFramework paginationView
Can also be written using get
:
renderedHtml = (get #styledPagination theCSSFramework) theCSSFramework paginationView
That's important to understand here. We get a styledPagination
function using get #styledPagination theCSSFramework
.
Then we apply theCSSFramework
to that function. We apply the theCSSFramework
to that function because the styledPagination
function internally might want to call other functions of the CSSFramework
type. But you might want to override some functions of the CSSFramework
.
Here's an example what would if we don't pass this a second time:
data CSSFramework = CSSFramework { styledPagination :: Html, styledButton :: Html }
bootstrapCSS = CSSFramework { styledPagination, styledButton }
where
styledPagination = [hsx|<div>{styledButton}</div>|]
styledButton = [hsx|<button style="color: red">button</button>|]]
myPage = [hsx|{styledPagination bootstrapCSS}|]
All good. But now as a user we want to override the button styling:
customCSS = bootstrapCSS { styledButton = [hsx|<button style="color: green">button</button>|]] }
myPage = [hsx|{styledPagination customCSS}|]
When we now render the myPage
we will get <div><button style="color: red">button</button></div>
(a red button, while our customCSS
specified it should be green).
We can fix this by late-binding the calls by manually passing around a CSSFramework. Here I added a CSSFramework
to all functions:
data CSSFramework = CSSFramework { styledPagination :: CSSFramework -> Html, styledButton :: CSSFramework -> Html }
bootstrapCSS = CSSFramework { styledPagination, styledButton }
where
styledPagination cssFramework = [hsx|<div>{get #styledButton cssFramework}</div>|]
styledButton cssFramework = [hsx|<button style="color: red">button</button>|]]
myPage = [hsx|{styledPagination bootstrapCSS bootstrapCSS}|]
Now we can customize it again:
customCSS = bootstrapCSS { styledButton = \cssFramework -> [hsx|<button style="color: green">button</button>|]] }
myPage = [hsx|{styledPagination customCSS customCSS}|]
I hope this clears things up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Super helpful, thanks - In fact I'll copy/ paste this explanation to CSSFramework.hs
@@ -155,6 +164,79 @@ instance Default CSSFramework where | |||
|
|||
styledSubmitButtonClass = "" | |||
|
|||
styledPagination :: CSSFramework -> PaginationView -> Blaze.Html |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mpscholten this default
implementation is very much Bootstrap framework, however I don't see much value at having a stripped of classes version, as it will never be used as-is. Do you agree?
So bootstrap
function will have no change, only tailwind
will
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another option that might be better is to break it down into smaller pieces, so bootsrap and tailwind will just have to define the classes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this default implementation is very much Bootstrap framework, however I don't see much value at having a stripped of classes version, as it will never be used as-is. Do you agree?
Agree
Another option that might be better is to break it down into smaller pieces, so bootsrap and tailwind will just have to define the classes.
I just checked the tailwind docs and it looks like tailwind has a completly different dom structure for pagination. So I'd go with the first approach and just make bootstrap the default
@@ -155,8 +165,91 @@ instance Default CSSFramework where | |||
|
|||
styledSubmitButtonClass = "" | |||
|
|||
styledPagination :: CSSFramework -> PaginationView -> Blaze.Html | |||
styledPagination _ paginationView = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add a couple of tests to the new functions? https://github.com/digitallyinduced/ihp/blob/master/Test/View/CSSFrameworkSpec.hs :)
Added tests. When merging probably better to squash commits, as so many commits in the PR are breaking |
@@ -40,7 +40,7 @@ Install Tailwind via NPM as usual: | |||
|
|||
```bash | |||
npm init | |||
npm add tailwindcss postcss autoprefixer | |||
npm add tailwindcss postcss autoprefixer @tailwindcss/forms |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes TW behave better with forms, so gets the Pagination look better as-well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good job! :)
This is a very naive start for #1123