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

Flop.Phoenix.pagination change to allow not to render next/prev links #270

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

oliverswitzer
Copy link

@oliverswitzer oliverswitzer commented Sep 28, 2023

Description

Thanks for making this awesome library!

This PR adds the ability to specify that you do not wanter to render next and previous links.

Background: we're experiencing a styling conflict with our UI library daisyUI that causes the nubmered pages left border radius to get cut off when next / prev links are rendered (which we do not want).

image

This feature allows you to disable rendering those buttons if they aren't wanted.

Checklist

  • [ x] I added tests that cover my proposed changes.
  • [ x] I updated the documentation related to my changes (if applicable).

* add ability to specify that you do not wanter to render next and previous links
@cadebward
Copy link

FYI. I was able to solve the same thing with the built in options. For example, I hid the page numbers with:

def pagination_opts do
  [
    pagination_list_attrs: [class: "hidden"],
  ]
end

@woylie
Copy link
Owner

woylie commented Oct 2, 2023

FYI. I was able to solve the same thing with the built in options. For example, I hid the page numbers with:

def pagination_opts do
  [
    pagination_list_attrs: [class: "hidden"],
  ]
end

Do you still think the changes in this PR would be useful, or are you happy with the CSS solution?

@coveralls
Copy link

coveralls commented Oct 2, 2023

Coverage Status

coverage: 100.0%. remained the same when pulling 322d121 on oliverswitzer:update-pagination-component into 12e6c15 on woylie:main.

@cadebward
Copy link

I am personally happy with the CSS solution, but curious to hear what @oliverswitzer thinks.

@oliverswitzer
Copy link
Author

oliverswitzer commented Oct 10, 2023

Hey friends, sorry, been a busy couple of weeks!

To be clear I wasn't hoping to hide page numbers entirely, which seems like what the solution @cadebward was recommending (pagination_list_attrs: [class: "hidden"]) would do, but instead fix the issue shown in the screenshot where the border radius of the left most pagination number is cut off due to the presence of the "previous" button in the list.

Even if I try to hide the previous and next buttons with setting next_link_attrs={[class: "hidden"]} and previous_link_attrs={[class: "hidden"]}, this issue still persists due to the element still being present in the DOM. daisyUI's join class selector (used in their pagination component) is treating the previous button as the first element that needs a border-radius left applied, even though it is technically "display: none"... As a result the actual first element that should be visible in the list (ie the 1st page button) is not getting the proper border radius. I hope that makes sense.

I think next/prev elements need to not be rendered in the DOM entirely to fix this issue.

@@ -186,7 +186,8 @@ defmodule Flop.Phoenix do
Default: `#{inspect(Pagination.default_opts()[:wrapper_attrs])}`.
"""
@type pagination_option ::
{:current_link_attrs, keyword}
{:render_next_and_previous_links, boolean()}
| {:current_link_attrs, keyword}
Copy link
Owner

Choose a reason for hiding this comment

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

Little nitpick, the options here are in alphabetical order, can you move the new option to the right position?

@@ -8,6 +8,7 @@ defmodule Flop.Phoenix.Pagination do
@spec default_opts() :: [Flop.Phoenix.pagination_option()]
def default_opts do
[
render_next_and_previous_links: true,
Copy link
Owner

Choose a reason for hiding this comment

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

This list is alphabetically sorted as well. 🔤

@woylie
Copy link
Owner

woylie commented Oct 12, 2023

Hey friends, sorry, been a busy couple of weeks!

To be clear I wasn't hoping to hide page numbers entirely, which seems like what the solution @cadebward was recommending (pagination_list_attrs: [class: "hidden"]) would do, but instead fix the issue shown in the screenshot where the border radius of the left most pagination number is cut off due to the presence of the "previous" button in the list.

Even if I try to hide the previous and next buttons with setting next_link_attrs={[class: "hidden"]} and previous_link_attrs={[class: "hidden"]}, this issue still persists due to the element still being present in the DOM. daisyUI's join class selector (used in their pagination component) is treating the previous button as the first element that needs a border-radius left applied, even though it is technically "display: none"... As a result the actual first element that should be visible in the list (ie the 1st page button) is not getting the proper border radius. I hope that makes sense.

I think next/prev elements need to not be rendered in the DOM entirely to fix this issue.

Alright, let's add the option. I just had two small comments.

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

Successfully merging this pull request may close these issues.

4 participants