Bug: Anchor_string does not apply to disabled anchors

ibrahima commented 1 month ago

👀 Before submitting...

[X] I upgraded to pagy version 9.0.5
[X] I searched through the Documentation
[X] I searched through the Issues
[X] I searched through the Q&A

🧐 REQUIREMENTS

[X] I am providing a VALID code file that confirms the bug
[X] I am NOT posting any of the USELESS THINGS listed above
[X] I am aware that this issue will be automatically closed if the code file is missing or INVALID

💬 Description

Hi! I'm just getting started with pagy, and I was trying to figure out how best to style it within the context of my application. I wanted to apply some classes that are already defined to the anchors. I noticed though that the anchor_string argument only applies to the non-disabled anchors, which isn't what I'd want personally. Is this intentional, or a bug? Thank you!

Here's my reproduction: https://gist.github.com/ibrahima/cc448df2be46b6c22095d9de3a9e2ffa#file-repro-ru

The relevant portion is

    /* My addition */
    .pagy .my-favorite-class {
      background-color: red !important;
    }

and

  <%= pagy_nav(@pagy, id: 'nav', aria_label: 'Pages nav', anchor_string: 'class="my-favorite-class"') %>

I was expecting in this example that all the anchors would be turned red, even the disabled ones.

I understand if this is intended behavior, I can figure out a different way to style things. I was just trying to understand if this was intentional. Thank you!

ddnexus commented 1 month ago

Hi @ibrahima, the current page was not an anchor in previous versions, so what was the anchor_string in that version was not included in the current page markup. The successive refactoring made the current page an anchor without href, but continued to ignore it in the markup.

That's an inconsistency which should be considered a bug, so this issue is valid.

Your particular example is good for highlighting the inconsistent behavior, however using the anchor_string to add a class is probably not the best way to do it.

You can target the anchors with a CSS rule, and with the classes keyword argument, which is currently ignored in the simple pagy_nav (which is another inconsistency, hence a possible bug to be fixed)

Will fix ASAP.

benkoshy commented 1 month ago

https://github.com/ddnexus/pagy/commit/92fd3453cc6d8d1ea3885858795c68dd31d03398 - all i did was add it in the markup for the anchor-string for the gaps and current pages.

Is this the correct approach?

I can easily do for the other nav helpers and add tests.

ddnexus commented 3 weeks ago

OK, so I finally had a bit of time to analyze the matter properly.

The main goal of the anchor_string is allowing to use data-* attributes. Its naming is generic enough so to cover any string that you may want to add to the actual page links, but that does not mean we should abuse it. Current page and gaps became disabled anchors only to get an easier and more consistent way to target them with CSS rules, but are actually NOT used as active page links.

Now, using anchor_string to target also the disabled links (that became links only to be consistently targeted by CSS rules) would make it technically consistent, but would add things that we don't need - exactly because now we can easily target them with CSS rules.

In conclusion the anchor_string seems better added only to the active links - as it was designed originally. That means that unless there is some request with a valid use-case that require the change, the code is fine as-is ATM.

On the other hand, the docs require changes to fix and clarify the matter.

Will do ASAP.

ibrahima commented 3 weeks ago

Thanks for looking into it!

ddnexus / pagy