Views Pager - 0 Items is confusing

Binoli Lalani → made their first commit to this issue’s fork.

Seems like a change that needs test coverage.

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"

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

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

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.

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 → .

Thanks @mradcliffe,
Updating the Issue summary.

Good catch @mradcliffe

Solution seems clearer now.

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.

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.