Some text fields in views pager forms that accepts only integer values have #textfield type. We can improve user experience by replacing them with #number form element and applying appropriate #min and #max properties.

CommentFileSizeAuthor
#26 2847921-26.patch7.42 KBkrzysztof domański
#21 use_number_form-2847921-21.patch7.41 KBchi
#21 interdiff-21-10.txt3.1 KBchi
#10 interdiff-2847921-4-8.txt1.92 KBjaykandari
#10 use_number_form-2847921-10.patch4.33 KBjaykandari
#8 use_number_form-2847921-8.patch4.33 KBjaykandari
#4 interdiff-2-4.txt1.97 KBshadcn
#4 2847921-4.patch2.32 KBshadcn
#2 2847921-2.patch1023 bytesshadcn
pager-options.png24.75 KBchi

Comments

Chi created an issue. See original summary.

shadcn’s picture

shadcn commented
Status: Active » Needs review
StatusFileSize
new 2847921-2.patch1023 bytes

Looks like this is missing for the Some plugin only.

chi’s picture

chi commented
Status: Needs review » Needs work

Since negative values make no sense for these fields I propose we set #min value for them to zero.

shadcn’s picture

shadcn commented
Version: 8.3.x-dev » 8.4.x-dev
Component: views_ui.module » views.module
Status: Needs work » Needs review
StatusFileSize
new 2847921-4.patch2.32 KB
new interdiff-2-4.txt1.97 KB
chi’s picture

chi commented
Title: Use "number" form element to specifiy a number of items in Views pager settings » Use "number" form element in Views pager settings form
Status: Needs review » Needs work

As you changed settings in the SqlBase class the scope of the issue is slightly extended. Actually I think it would be better. So let's fix all Views pager plugins at once.

Here are the elements that needs to be fixed after applying patch #4.

Plugin: "Display all items":
"Offset (number of items to skip)" can be turned to number with #min = 0.
Plugin: "Paged output, full pager":
"Pager ID" and "Number of pager links visible" cannot be negative.
Plugin: "Paged output, mini pager":
"Pager ID" cannot be negative.
chi’s picture

chi commented

Pager ID cannot be negative.

Well, I would like someone to confirm this statement.

jaykandari’s picture

jaykandari
Primary language Hindi
commented
Assigned: Unassigned » jaykandari
jaykandari’s picture

jaykandari
Primary language Hindi
commented
Assigned: jaykandari » Unassigned
Status: Needs work » Needs review
StatusFileSize
new use_number_form-2847921-8.patch4.33 KB

Extended #4 to include changes mentioned in #5.

shadcn’s picture

shadcn commented

Thanks @JayKandari. Can you please include an interdiff?

jaykandari’s picture

jaykandari
Primary language Hindi
commented
StatusFileSize
new use_number_form-2847921-10.patch4.33 KB
new interdiff-2847921-4-8.txt1.92 KB

Sure !
Resubmitted #8 & an interdiff from #4.
Thanks !

chi’s picture

chi commented
Status: Needs review » Reviewed & tested by the community

This looks fine for me. Thanks.

cilefen’s picture

cilefen commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs issue summary update, +Needs tests

I like this issue! +1

The scope of the issue was expanded in #5—rightly, those are bugs—but the issue summary and title have not changed. Please update it. Test are a core gate. We need some here in order to prevent this happening again.

Manually testing this patch made me notice that Views doesn't validate this input at all. It turns out, courtesy of xjm, there is already an issue open for that: #2392823: [meta] Much Views UI input is not validated. I opened #2853054: Add validation constraints for views pager config to address pagers specifically.

Thank you!

chi’s picture

chi commented

@cilefen, what kind of tests are expected here? Shall we load the form objects and check #type property of these elements? Could you please point out existing tests that check type of form elements?

cilefen’s picture

cilefen commented

Drupal\views\Tests\Plugin\PagerTest and you can probably do an xpath search to verify the element type.

chi’s picture

chi commented
Issue tags: -Novice

This approach sounds too formal. The PagerTest will be inconsistent. If we test '#type' = 'number' we should test all other types and perhaps not just types but any kind of form properties as well (#title, #options, etc).

cilefen’s picture

cilefen commented

What do you suggest?

chi’s picture

chi commented

I would just give this up at least in this patch. We could create a followup to improve the PagerTest but it does not much differ from tons of other core tests which deal with forms. As far as I have learned we generally don't care about testing form elements in details.

cilefen’s picture

cilefen commented

The real problem, in this case, is #2853054: Add validation constraints for views pager config. It's up to you if you want to re-RTBC this and see if the other committers agree with you. It's a simple enough change. But I can't apply my own discretion in a provisional role.

chi’s picture

chi commented
Issue summary: View changes
Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs issue summary update

Ok. Let's check what other committers will say about this.

xjm’s picture

xjm
she/her
Primary language English
commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs manual testing

I agree that we need automated test coverage: https://www.drupal.org/core/gates#testing We are adding a minimum value here, so we should both ensure that our automated tests test that expected UI input leads to expected results (and simply document/demonstrate if so) and also add test coverage that now values lower than the minimum aren't accepted. It can be a fairly simple addition to the existing test, probably just an assertion or two. The goal is not to test the Form API, just the pager UI basic functionality.

Simple manual testing and a screenshot showing the UI change would also be nice here, so tagging for that too.

I agree also with scoping the validation issue to a followup. That's definitely something that needs to be addressed in Views, but not in scope for this small UX improvement.

Thanks @cilefen and @Chi!

chi’s picture

chi commented
Status: Needs work » Needs review
StatusFileSize
new interdiff-21-10.txt3.1 KB
new use_number_form-2847921-21.patch7.41 KB

Added a test.

probably just an assertion or two

So far we have 12 number elements in pager settings forms. I've added assertions for each one.

Here is a quick inventory:

Plugin: "Display a specified number of items"
Items per page
Offset
Plugin: "Display all items"
Offset
Plugin: "Paged output, full pager"
Items to display
Offset
Pager ID
Number of pages
Number of pager links
Plugin: "Paged output, mini pager"
Items per page
Offset
Pager ID
Number of pages

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

joachim’s picture

joachim commented
Status: Needs review » Reviewed & tested by the community

LGTM.

krzysztof domański’s picture

krzysztof domański
Location Poland
commented
Status: Reviewed & tested by the community » Needs review
StatusFileSize
new 2847921-26.patch7.42 KB

Correcting a patch #21 that is out of date for 8.7.x-dev.

msankhala’s picture

msankhala commented
Status: Needs review » Reviewed & tested by the community

LGTM

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 26: 2847921-26.patch, failed testing. View results

Anonymous’s picture

Anonymous (not verified) commented
Status: Needs work » Reviewed & tested by the community

Unrelated error #28. Test pass. Mark again as RTBC.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed bcfce36 and pushed to 8.7.x. Thanks!

  • alexpott committed bcfce36 on 8.7.x 
    Issue #2847921 by JayKandari, arshadcn, Chi, Krzysztof Domański, cilefen...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.