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

allow any HTML attributes on any links #417

Closed
wants to merge 2 commits into from

Conversation

leckylao
Copy link

@leckylao leckylao commented Jul 2, 2013

@billychan @zzak @yuki24 @amatsuda

As everything we want to add in are supported natively by the Rails link_to helper. So what we need to do is just have one attribute called :options.

So in _first_page.html.erb it becomes:

  <%= link_to_unless current_page.first?, raw(t 'views.pagination.first'), url, {remote: remote}.merge(options||={}) %>

Then you can add any attributes you want:

  <%= paginate @users, options: {remote: true, class: 'ajax_index', data: {pjax: true} } %>

In this way, it will not break the existing code and can still achieve what we need. And the PR title can be changed to "allow any HTML attributes on any links" 😄

By using this way, we can even remove the original remote: remote as by default link_to helper :remote is false. Much cleaner.

So it becomes:

  <%= link_to_unless current_page.first?, raw(t 'views.pagination.first'), url, options||={} %>

This PR only added in the options support.

@leckylao
Copy link
Author

leckylao commented Jul 2, 2013

#411

@billychan
Copy link

Update: I myself still find using 'remote', 'data' at first level sounds natural to me, as that is how 'link_to' handle params.

@yuki24
Copy link
Member

yuki24 commented Jul 12, 2013

All the tests failed. Have you confirmed that this PR really works? I really can't merge anything that break things.

@yuki24 yuki24 closed this Jul 12, 2013
@leckylao
Copy link
Author

Hi @yuki24 , this PR works and I'm using it for my work project. U can see the changes are only view changes. Test fail can be fix if u are telling me instead of closing the issue. By closing it I assume you don't like this feature. So I will just use locally. But I think it's a good refactor on the helper API for kaminari.

@zzak
Copy link
Contributor

zzak commented Jul 15, 2013

I actually like this idea, but it does need tests.

Also, since it's an API breaking change, we will have to release it in a future version.

@zzak zzak reopened this Jul 15, 2013
@yuki24
Copy link
Member

yuki24 commented Mar 29, 2014

I would like to refine the implementation/idea of this request and merge it into master. Could you review the following idea?

  • Add html option that will be passed to any link tags(following form_for's convension)

    paginate @users, html: {remote: true, class: 'ajax_index', data: {pjax: true} }
  • Deprecate remote option in favor of the above

  • Other tags like <nav> and <span> won't be given html attributes

@yuki24 yuki24 modified the milestone: 1.0.0 Apr 1, 2014
@amnesia7
Copy link

Thanks for the suggestion. This made it easier to apply data attributes to page links for using wiselinks gem.

…or's convension)

paginate @users, html: {remote: true, class: 'ajax_index', data: {pjax: true} }
Deprecate remote option in favor of the above
Other tags like <nav> and <span> won't be given html attributes
@leckylao
Copy link
Author

leckylao commented May 8, 2014

hi @yuki24 @zzak , I've updated as @yuki24 suggested.

@MatrixFr
Copy link

@yuki24 this suggestion i not include in official gem, can you merge and update gem?

@mjrk
Copy link

mjrk commented Dec 3, 2014

I added a version with tests for this feature: https://github.com/amatsuda/kaminari/pull/628. Please let me know if you like it.

@mjrk mjrk mentioned this pull request Dec 4, 2014
@swaathi
Copy link

swaathi commented May 18, 2015

Hi. I'd like to know when this option would be available in Kaminari. I'm using Wiselinks gem, and I'd like to have the data-push attribute in the pagination link. Thanks!

@yuki24
Copy link
Member

yuki24 commented May 23, 2015

@swaathi take a look at this answer on Stackoverflow.

I'm actually closing this issue because kaminari already provides a way to cover any edge cases. Usually editing templates only takes less than 5min and it's super simple. I really don't want to add any more features that only save us a small amount of time but makes it harder to maintain the codebase.

We might remove the remote option since there's no reason to specialcase it.

@yuki24 yuki24 closed this May 23, 2015
@swaathi
Copy link

swaathi commented May 23, 2015

Thank you! I actually realized it's quite simple if you dig into the code. I used this tutorial to help me out,
http://blog.berylliumwork.com/2013/08/wiselinks-with-rails-4.html?m=1. Maybe someone will find it useful :)

On May 23, 2015, at 10:06 PM, Yuki Nishijima notifications@github.com wrote:

@swaathi take a look at this answer on Stackoverflow.

I'm actually closing this issue because kaminari already provides a way to cover any edge cases. Usually editing templates only takes less than 5min and it's super simple. I really don't want to add any more features that only save us a small amount of time but makes it harder to maintain the codebase.

We might remove the remote option since there's no reason to specialcase it.


Reply to this email directly or view it on GitHub.

@gosukiwi
Copy link

@yuki24 while you can do lots of customizations by modifying the views, whatever you change, will be changed for all instances of pagination, which might not be what you want.

I really think it would be great to be able to pass an options hash for HTML attributes just like standard Rails does. A use case I have right now is that I'm displaying a Turbo Frame with some content. I don't want the links in the frame to be rendered inside the frame, but I do want the paginations links to do so.

Normally one would do it like this:

= turbo_frame_tag :posts, target: '_top'
  - @posts.each do |post|
    link_to post.title, post
  link_to 'Next Page', some_url, data: { turbo_frame: :posts }

While you could use themes and templates, you'd need one theme for each different turbo frame you want to work with. Something like:

= paginate @posts, data: { turbo_frame: :posts }

Would be ideal, IMO. I think there are more use-cases for this besides the one I showed.

@yuki24
Copy link
Member

yuki24 commented Jun 23, 2021

I would recommend maintaining your own themes and implement it yourself. These days almost no apps go production with Kaminari's default templates since we all have a design system or a style guide. Running rails g kaminari:views default is a good starting point. You can refer to https://github.com/kaminari/kaminari/pull/628/files as an example implementation. As you can see, there is no secret in customizing templates and you don't need any changes in Kaminari to make it.

Note that there are a few considerations you should keep in mind.

  1. All keyword parameters passed to paginate become available as a local variable (or local_assigns more specifically) in the kaminari templates. What this means is that there is a risk of naming conflict if you choose a commonly used HTML attribute names.
  2. There are elements that are not a clickable link element (e.g. a gap element), or some design systems choose to disable pagination links rather than hiding when necessary. Depending on the JS behaviour you are attaching to the DOM elements, globally passing in the same attributes to all elements may lead to unexpected behaivour.

In terms of priority, we would like to prioritize features that absolutely require internal changes in Kaminari (e.g #938) over nice-to-have feature requests like this.

@gosukiwi
Copy link

Thanks for taking the time to reply. It makes sense.

All keyword parameters passed to paginate become available as a local variable

I didn't know that was the case. Maybe I'm dumb and missed it in the README, but if that's not the case, it might be a good idea to add it there :)

Cheers, and thanks for taking the time to maintain this awesome gem.

@rctneil
Copy link

rctneil commented Mar 14, 2022

I'm currently in need of exactly this. I need all links in just ONE pagination to have "data-turbo-action" attribute on them.

How can I accomplish this?

@yuki24
Copy link
Member

yuki24 commented Mar 15, 2022

@rctneil I'd recommend generating the default pagination views:

$ rails g kaminari:views default

From there, you have the freedom of customizing anything and making it behave exactly as you expect it to.

@rctneil
Copy link

rctneil commented Mar 15, 2022

@yuki24 hi. Yes I have already done that but I'm this case, I only wish this to apply to one set of pagination links on the site, not all of them

Also by using that I have to add the data attribute to 5 files which to me appears to go against the whole don't repeat yourself convention.

I just need a way to apply this to one single paginate()

Thanks

@yuki24
Copy link
Member

yuki24 commented Mar 15, 2022

I only wish this to apply to one set of pagination links on the site, not all of them

I'm not sure I understand this. Could you elaborate?

Also by using that I have to add the data attribute to 5 files which to me appears to go against the whole don't repeat yourself convention.

DRY doesn't mean you shouldn't duplicate code. This was explicitly stated by Dave Thomas, the author of the book Pragmatic Programmer.

Most people take DRY to mean you shouldn't duplicate code. That's not its intention. The idea behind DRY is far grander than that.

DRY says that every piece of system knowledge should have one authoritative, unambiguous representation. Every piece of knowledge in the development of something should have a single representation. A system's knowledge is far broader than just its code. It refers to database schemas, test plans, the build system, even documentation.

@rctneil
Copy link

rctneil commented Mar 15, 2022

@yuki24 ok. I take your comment on DRY. Got it!

What I meant by the views is that those views are used every time that the paginate view helper is used on the site. I only want the data attribute mentioned above to appear on one pagination control. So where pagination exists on other pages I don't want that data attribute. Therefore I can't just add it to the kaminari views. In this case it's just a one off occurrence.

I hope that makes more sense!

@yuki24
Copy link
Member

yuki24 commented Mar 15, 2022

Thanks for the clarification. You may be able to pass in extra params like this:

<%= paginate @posts, data: { turbo_frame: :posts } %>

And then try using **local_assigns.slice(:data) in the pagination views like app/views/kaminari/_page.html.erb, so the :data argument behaves like an optional argument:

<%= link_to_unless page.current?, page, url, remote: remote, rel: page.rel, **local_assigns.slice(:data) %>

@rctneil
Copy link

rctneil commented Mar 15, 2022

@yuki24 Thankyou!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants