Views Pager - 0 Items is confusing

Created on 20 November 2020, over 3 years ago
Updated 24 May 2024, about 2 months ago

Problem/Motivation

While creating a view, user requested to see all records on 1 page. The previous pager setting was 'Display a specified number of items" and a value of 10.

I left the pager type as it was and set the pager value to '0' (which Views field Items Per Page 'help' text suggests). The result on the views screen says "Display a specific number of items | 0 items". I completely understand why and how but for human readability it is nonsensical. When the value is '0' this should exception to 'All' items.

Steps to reproduce

Create a view.
Set Pager to : Use Pager: Display a specified number of items. Accept the defaults (10 items with skip of 0).
From view click on the pager '10 items' hyperlink and set the value to 0.

Proposed resolution

If the user sets the pager type to "Display number of items" and

  1. offset is set to 0, the number items is set to 0, display text string 'All' instead of numeric 0.
  2. offset is set to 0, the number items is set is not 0, display text string 'All items except first @offset'.

For all the other combinations, the display text on the pager option link remains unchanged.
This does not change the existing logic and behaviour of the pagination which and also clears the confusion about the behaviour.

Remaining tasks

See the tags and the MR
Followup for #24

User interface changes

Before

After

API changes

Data model changes

Release notes snippet

๐Ÿ› Bug report
Status

Needs work

Version

11.0 ๐Ÿ”ฅ

Component
Views UIย  โ†’

Last updated 4 days ago

Created by

๐Ÿ‡บ๐Ÿ‡ธUnited States jshimota01 Portland, OR

Live updates comments and jobs are added and updated live.
  • Novice

    It would make a good project for someone who is new to the Drupal contribution process. It's preferred over Newbie.

  • Usability

    Makes Drupal easier to use. Preferred over UX, D7UX, etc.

  • Needs tests

    The change is currently missing an automated test that fails when run with the original code, and succeeds when the bug has been fixed.

Sign in to follow issues

Merge Requests

Comments & Activities

Not all content is available!

It's likely this issue predates Contrib.social: some issue and comment data are missing.

  • ๐Ÿ‡ฎ๐Ÿ‡ณIndia djsagar
  • ๐Ÿ‡ฎ๐Ÿ‡ณIndia Binoli Lalani Gujarat

    Binoli Lalani โ†’ made their first commit to this issueโ€™s fork.

  • Pipeline finished with Failed
    2 months ago
    Total: 638s
    #165508
  • Status changed to Needs review 2 months ago
  • ๐Ÿ‡ฎ๐Ÿ‡ณIndia Binoli Lalani Gujarat
  • Status changed to Needs work 2 months ago
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States smustgrave

    Seems like a change that needs test coverage.

  • ๐Ÿ‡ฎ๐Ÿ‡ณIndia sushyl@gmail.com Pune

    Keeping the novice tag on this one after reviewing the issue.
    This needs a simple assertion which can be added in the existing test SorePagerSettings to assert that when
    - the number of items in the pager is set to `0`
    - and the pager type is set to "Display a specified number of items"
    the text on the pager option link is "All items" and not "0 items"

  • ๐Ÿ‡บ๐Ÿ‡ธUnited States ian.ssu

    My name is Ian and I'm working today on this issue at DrupalCon 2024

  • Pipeline finished with Failed
    2 months ago
    Total: 393s
    #168091
  • Pipeline finished with Failed
    2 months ago
    Total: 508s
    #168144
  • Pipeline finished with Canceled
    2 months ago
    Total: 333s
    #168152
  • Pipeline finished with Success
    2 months ago
    #168165
  • Status changed to Needs review 2 months ago
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States ian.ssu

    I think I added the suggested test. Forgive the extra commits I was unable to get failing tests with DrupalPod.

  • Status changed to RTBC 2 months ago
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States smustgrave

    No need to apologize. Tests were exactly what I was thinking

    Ran test-only feature of gitlab https://git.drupalcode.org/issue/drupal-3183766/-/jobs/1550667

    1) Drupal\Tests\views\Functional\Plugin\PagerTest::testStorePagerSettings
    Behat\Mink\Exception\ResponseTextException: The text "All items" was not found anywhere in the text of the current page.
    /builds/issue/drupal-3183766/vendor/behat/mink/src/WebAssert.php:907
    /builds/issue/drupal-3183766/vendor/behat/mink/src/WebAssert.php:293
    /builds/issue/drupal-3183766/core/tests/Drupal/Tests/WebAssert.php:975
    /builds/issue/drupal-3183766/core/modules/views/tests/src/Functional/Plugin/PagerTest.php:89
    /builds/issue/drupal-3183766/vendor/phpunit/phpunit/src/Framework/TestResult.php:729
    ERRORS!
    

    Which shows the issue. Manually testing also seeing the issue.

    Change fixes the issue for me so believe this one is good.

  • Status changed to Needs work 2 months ago
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States mradcliffe USA

    Thank you for adding the tests and reviewing the issue @ian.ssu, @sushyl.

    Thank you for reviewing the added tests and manually testing, @smustgrave.

    I read the issue summary, but it was not clear to me what among the 3 proposed resolutions was the proposed resolution that was RTBC. I changed the status to Needs work only so that we can update the issue summary to clarify the proposed resolution.

    The text changes should also be mentioned under User interface changes per Issue summary field - Issue Summary Template โ†’ .

  • Status changed to Needs review 2 months ago
  • ๐Ÿ‡ฎ๐Ÿ‡ณIndia sushyl@gmail.com Pune

    Thanks @mradcliffe,
    Updating the Issue summary.

  • Status changed to RTBC 2 months ago
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States smustgrave

    Good catch @mradcliffe

    Solution seems clearer now.

  • Status changed to Postponed about 2 months ago
  • ๐Ÿ‡ณ๐Ÿ‡ฟNew Zealand quietone New Zealand

    Thank you to everyone at Portland for working on this issue!

    I am triaging the RTBC queue. I began by reading the title, Issue Summary and then the last few comments. The title could be improved, it should indicate something about the change. We need to consider that this is used in the commit message and needs to make some sense to someone doing some git archeology. #41 asked for changes to the Issue Summary in the User Interface section. That section is empty so that has not been completed. Also, this is making changes to the User Interface so before and after screenshots should be available from the Issue Summary. The latest screenshots here are from 2 years ago.

    Issues that are changing the UI are to be tagged 'usabilty', โ†’ I am adding that tag. Usability issues often need a usabilty review as well. That may not been needed but without screenshots I am not able to make a decision.

    I then went back and reviewed the code and tested locally. While doing so I made screenshots and have updated the issue summary. The changed code does what it needs to and is working well. Yay! I did spot that the test is only testing the changes to the 'some' pager type. It should test all types and all text changes.

    While the tests were running I read the comments. I see that #4 is addressed but I don't think that #24 has been. That is a totally different approach. It is also a reminder to us all to read all the comments first!

    I am going to save the changes I made to the MR but set this postponed and ask the other committers.

  • Status changed to Needs work about 2 months ago
  • ๐Ÿ‡ณ๐Ÿ‡ฟNew Zealand quietone New Zealand

    After some time away and eating a meal I concluded there is no need for consultation. The way forward is to open an new issue to look into the change suggested in #24. This issue improves the user interface and I think that is more important than an ideal improvement, even if that improvement means removing the code added here.

Production build 0.69.0 2024