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.

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

Foundation extra #79

Merged
merged 9 commits into from
Aug 18, 2018
Merged

Foundation extra #79

merged 9 commits into from
Aug 18, 2018

Conversation

dwieringa
Copy link
Contributor

Adds support for Zurb Foundation 6

@dwieringa
Add pagy_nav_foundation helper
ce9c34b
@dwieringa
Remove Foundation's pagination-previous and pagination-next classes
0d91895 
These classes add Foundation's pagination arrows to the Next and Previous links.
I'm removing the classes since they don't do anything else and pagy already
supplies arrows via `pagy_t` and the I18n dictionary.
@dwieringa
Add foundation templates
cb2241e
@dwieringa dwieringa mentioned this pull request Aug 7, 2018
Closed
@ddnexus
Copy link
Owner

ddnexus commented Aug 7, 2018

Thank you! I will look at this ASAP.

@dwieringa
Copy link
Contributor Author

Something I'm wondering about now... I removed the prev and next classes since I was following the Foundation example (https://foundation.zurb.com/sites/docs/pagination.html) and assumed those were for Bootstrap. Let me know if those should be included as part of pagy.

Also, last night I took a first pass at adding the compact and responsive versions. The layout is not right on the compact version and I'm not sure I'll be able to put much more effort in this week. Do you want me to push this into this PR or wait until I have time to look deeper?

@ddnexus
Copy link
Owner

ddnexus commented Aug 7, 2018

I see you are making progress! The prev and next classes do nothing but are there just to add some easy selector in case a user may need it. They are present in all the helpers, so you may want to restore them.
About the compact layout... that's the most custom component, hence a bit more tricky to get it right for all the frameworks. Please, push it as it is, if I have some time I may setup a little app and try to help. (if you already have a test app with foundation included, please, let me know so I may save time. Thanks.

@dwieringa
WIP: Add compact and responsive extras for foundation
3a6e3d1 
Layout on `compact` needs work

I need to add the `next` and `pre` classes back to all foundation helpers
@dwieringa
Add prev and next html classes back in for Foundation extra
e6bdb21
@dwieringa
Copy link
Contributor Author

Ok, I added the prev and next classes back in. I also committed my compact and responsive extras even though compact needs some layout work.

I don't currently have a simple test app -- I've just been testing inside my client project where I was integrating pagy.

By the way, I haven't tested the haml template yet since I don't use it. I did however compare the output of slim and erb against the helper method.

@ddnexus
Copy link
Owner

ddnexus commented Aug 7, 2018

Thank you!

@dwieringa
Add foundation to initializer_example.rb
2a8c08f
@dwieringa
Add role back in for foundation extra
1c7909b
@dwieringa
Remove link.call from current page reference
220373e 
`link.call` was inserting the a anchor tag to the current page, which
isn't needed.

Having the anchor tag within the `current` `li` was also causing issues
with Zurb Foundation styling.  The `current` background was too large and
the text was showing up black instead of white.
@ddnexus
Copy link
Owner

ddnexus commented Aug 16, 2018

Hi @dwieringa,
I will probably have time to merge this PR during the weekend. What is the status? Just the compact is missing or incomplete?

@dwieringa
Copy link
Contributor Author

Hi @ddnexus,

I'm using the helper in production now and it seems to be working well.
I've compared the erb and slim template output to the helper (but not the haml template).

Correct, in the compact extra, the Foundation helper needs work (primarily with layout, which isn't my specialty). My testing of the responsive helper was minimal.

One thought I've had a few times (but I don't have a strong opinion)... would it be better for the compact and responsive helpers to be moved to the respective frontend extra's? Then the user would choose the frontend extra (eg foundation) and get the default+compact+responsive helpers...vs choosing compact and getting default+bootstrap+bulma+foundation+materialize+whatever-comes-later. This would be a fairly large reorganization though and probably beyond the scope of this PR.

I haven't updated the compact and responsive docs yet.

@ddnexus
Copy link
Owner

ddnexus commented Aug 17, 2018

Hi @dwieringa,
thanks for your answer. Yeah, what you are suggesting has been in my mind for a while. There are pros and cons for the 2 types of organizations... however the more new frameworks are coming the more logical would be that change. I will probably go back to this topic as soon as I will find some time.

@ddnexus ddnexus merged commit 6681b62 into ddnexus:dev Aug 18, 2018
ddnexus pushed a commit that referenced this pull request Aug 18, 2018
@dwieringa @ddnexus
Foundation extra (#79)
d78f85d 
Add pagy_nav_foundation helpers and templates
@ddnexus
Copy link
Owner

ddnexus commented Aug 18, 2018

Added a few things (ba7d046): now it should be complete and work well. Thanks.

@ddnexus ddnexus added the merged label Aug 19, 2018
@ddnexus
Copy link
Owner

ddnexus commented Aug 21, 2018

@dwieringa
FYI: all the extras have been reorganized: the compact and responsive navs have been moved into the respective extras. Thanks

@dwieringa
Copy link
Contributor Author

Thanks @ddnexus. The reorganization looks good. I switched to using your latest release.

The only thing I noticed is a copy/paste error on the foundation docs page (https://ddnexus.github.io/pagy/extras/foundation). It states "This extra adds one nav helpers to the Pagy::Frontend module", but should probably say "adds three nav helpers", like bootstrap does.

@ddnexus
Copy link
Owner

ddnexus commented Aug 22, 2018

oh... yes. good catch! Thanks.

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
merged
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@dwieringa @ddnexus