Skip to content

Always Show Pagination Overview#5591

Merged
danharrin merged 2 commits into
filamentphp:2.xfrom
intrepidws:always-show-pagination-totals
Feb 14, 2023
Merged

Always Show Pagination Overview#5591
danharrin merged 2 commits into
filamentphp:2.xfrom
intrepidws:always-show-pagination-totals

Conversation

@intrepidws
Copy link
Copy Markdown
Contributor

@intrepidws intrepidws commented Feb 1, 2023

When a resource has only one result, the pagination overview text (Showing X to Y of Z results) is not shown. While it's obviously pretty easy to see that there is only one record, I find that the change in the UI is jarring. There really isn't a great reason not to show it IMO.

This PR shows the pagination overview regardless of how many results there are.

@what-the-diff
Copy link
Copy Markdown
Contributor

what-the-diff Bot commented Feb 1, 2023

  • The trans_choice() method was used instead of the __() helper.
  • A new parameter for the total number of items in pagination overview was added to make it possible to use a plural form if needed (e.g., "1 item" vs "5 items").

@zepfietje zepfietje added this to the v2 milestone Feb 1, 2023
@zepfietje zepfietje added the ui label Feb 1, 2023
@zepfietje
Copy link
Copy Markdown
Member

We might want to update the string as it will now read "Showing 1 to 1 of 1 results"?

@intrepidws
Copy link
Copy Markdown
Contributor Author

Yea, it's certainly a bit awkward. What about just Showing 1 result?

@zepfietje
Copy link
Copy Markdown
Member

That would be better I guess.
Maybe that's the reason why it was hidden for 1 record in the first place? 😅

@intrepidws
Copy link
Copy Markdown
Contributor Author

If you're showing 10 records per page and there are 9 records available, the overview text shows but says Showing 1 to 9 of 9 results. So there's awkward sounding duplication there as well. In an ideal world, it would probably just say Showing 9 results.

While I'm willing to update this PR to account for the one page (Showing 9 results) and one record (Showing 1 result) scenarios, it would probably mean adding two new keys to every table language file, like this:

    'pagination' => [

        'label' => 'Pagination Navigation',

        'overview' => [
        
            'default' => 'Showing :first to :last of :total results',

            'one_page' => 'Showing :total results',

            'one_result' => 'Showing :total result',

        ],

I think that I could pretty easily figure out the wording for the one page scenario out for non-English languages. For instance, this:

'default' => 'عرض :first إلى :last من :total النتائج',

Would probably just become

'one_page' => 'عرض :total النتائج',

But the one result scenario would likely be implausible for many languages, since many are likely much more complicated than simply dropping the s off of results.

My vote would probably be to deal with the awkwardness of the rarely seen Showing 1 to 1 of 1 results and call it a day. I think slightly awkward language is better than a UI that changes unexpectedly.

@zepfietje
Copy link
Copy Markdown
Member

zepfietje commented Feb 2, 2023

In my opinion, if we're going to make any changes to this, we should fix the translations first instead of showing an awkward string.

We probably don't even need new localization strings, since we can use pluralization.
Have a look and let me know if you can get this working in a nice way.

Mark the PR as ready when done so we can have another look. 😊

@zepfietje zepfietje marked this pull request as draft February 2, 2023 07:40
@zepfietje zepfietje self-assigned this Feb 14, 2023
@zepfietje
Copy link
Copy Markdown
Member

I've updated this PR to use Showing 1 result.
It still uses Showing 1 to 9 of 9 results though, since changing that would require new translation keys, which are simply more of a hassle.

Still not too sure if this change is worth it, as this will now show Showing 1 to 1 of 1 results for locales that have not been updated to include this change.

What do you think, @danharrin?

@zepfietje zepfietje marked this pull request as ready for review February 14, 2023 09:56
@zepfietje zepfietje requested a review from danharrin February 14, 2023 09:56
@zepfietje zepfietje assigned danharrin and unassigned zepfietje Feb 14, 2023
@intrepidws
Copy link
Copy Markdown
Contributor Author

This feels like the right solution to me. As long as @danharrin is OK with it, I'll try and update more language files.

Thanks for getting to this @zepfietje, I've been massively sick for the last couple weeks.

@danharrin danharrin merged commit c424c36 into filamentphp:2.x Feb 14, 2023
@danharrin
Copy link
Copy Markdown
Member

Thanks, hope you're starting to feel better @intrepidws

zepfietje added a commit that referenced this pull request Feb 15, 2023
@intrepidws intrepidws deleted the always-show-pagination-totals branch December 29, 2023 14:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants