Problem/Motivation

Using a select list on a Number component with stepped values may result in the "max" value not being shown as a selection option.

Steps to reproduce

  1. Create or edit an existing webform
  2. Add a Number component and edit its options
  3. Set "minimum" to 0
  5. Set "maximum" to 22
  7. Set "step" to 10
  9. Under "Display" set "Element type" to "Select list"
  10. Save the component and view the webform

Expected: The last option in the select list is "22"
Actual: The last option in the select list is "20"

Proposed resolution

Add a checkbox option next to "step" to add the "Max" value, even if it is not a natural step.

Remaining tasks

The attached patch enables the functionality across the board, using existing commented-out code. This needs to be built out to be an option configurable on a per-component basis.

User interface changes

Adding a new configuration option for the Number component.

API changes

Unknown

Data model changes

Unknown

CommentFileSizeAuthor
#27 3016408-27.patch760 byteslucassc
#20 3016408-20.patch698 bytessophiavs
#2 webform-number_select_separator_fix-3016408.patch1.16 KBddiestra
#5 webform-include_max_on_step_and_key_by_value-3016408-5.patch1.34 KBcoderbrandon
#9 max_value_in_stepped_number_selector-3016408-9.patch701 bytesjoshf
#15 Captura de Tela 2022-04-04 às 15.12.32.png28.96 KBJohnny Santos
#19 3016408-19.patch436 bytessophiavs

Issue fork webform-3016408

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

ddiestra created an issue. See original summary.

ddiestra’s picture

ddiestra commented
StatusFileSize
new webform-number_select_separator_fix-3016408.patch1.16 KB
liam morland’s picture

liam morland
Primary language English
Location Ontario, CA 🇨🇦
commented
Title: Number selector separator fix. » Number selector separator fix
Status: Active » Needs review

Please explain what the problem is and what the patch is doing to fix it.

Status: Needs review » Needs work

The last submitted patch, 2: webform-number_select_separator_fix-3016408.patch, failed testing. View results

coderbrandon’s picture

coderbrandon commented
StatusFileSize
new webform-include_max_on_step_and_key_by_value-3016408-5.patch1.34 KB

Rerolling this for the newest version. This patch is mostly because of client requirements but I could see the value of having these changes being something configurable on the number component.

liam morland’s picture

liam morland
Primary language English
Location Ontario, CA 🇨🇦
commented

My question in #3 remains.

coderbrandon’s picture

coderbrandon commented

@Liam - There are 2 changes:
1. Include the max value even if it doesn't exactly match to the step
2. Key the options by the exact value instead of the numeric value. Ex: "1000" becomes "1,000.00" based on the component's configuration.

FYI, this isn't in any state to be merged into the module. There would need to be some additional work to make these changes configurable on the component itself instead of being blindly applied.

liam morland’s picture

liam morland
Primary language English
Location Ontario, CA 🇨🇦
commented
Category: Task » Feature request

Those two items should probably be separate issues.

joshf’s picture

joshf commented
Title: Number selector separator fix » Add max value in stepped number selector
Issue summary: View changes
StatusFileSize
new max_value_in_stepped_number_selector-3016408-9.patch701 bytes

Here is a patch for change 1 in Brandon's comment #7.

joshf’s picture

joshf commented

I opened #3215936 for change 2 in #7.

luisrhaas’s picture

luisrhaas commented
Status: Needs work » Needs review

Hi!

I think joshf did the remain tasks, I changed to "needs review".

Liam, if you need something more please let us know.

Thank you

Johnny Santos’s picture

Johnny Santos commented
Assigned: Unassigned » Johnny Santos

Going for the review

liam morland’s picture

liam morland
Primary language English
Location Ontario, CA 🇨🇦
commented

This should be put into an issue fork and merge request.

Johnny Santos opened merge request !167

Johnny Santos’s picture

Johnny Santos commented
Assigned: Johnny Santos » Unassigned
Status: Needs review » Reviewed & tested by the community
StatusFileSize
new Captura de Tela 2022-04-04 às 15.12.32.png28.96 KB

I just tested and its working as intended, screenshot added.
I made a issue fork and the merge request, as asked by Liam.

Johnny Santos’s picture

Johnny Santos commented
Status: Reviewed & tested by the community » Needs work

I'm moving back to 'needs work, since the tests are not passing.
I' dont know the reason since I didnt change anything from the patch.

BTW: drupal 9 has the same issue happening with max value and the steps.

liam morland’s picture

liam morland
Primary language English
Location Ontario, CA 🇨🇦
commented
Issue tags: +Need tests

Thanks for the merge request.

There is a coding standards problem (indentation) introduced by this change.

This needs tests.

Sometimes tests on merge requests falsely fail. Instead, a patch needs to be uploaded. This is reported in #3194156: Patches and Merge Requests lead to different test results.

sophiavs’s picture

sophiavs commented
Assigned: Unassigned » sophiavs

Hi, i'll be working on those coding standards issues and submit the new patch.

sophiavs’s picture

sophiavs commented
StatusFileSize
new 3016408-19.patch436 bytes

Testing if it'll pass on the tests

sophiavs’s picture

sophiavs commented
StatusFileSize
new 3016408-20.patch698 bytes
sophiavs’s picture

sophiavs commented
Assigned: sophiavs » Unassigned
Status: Needs work » Needs review

As the code with the standarts passed on the tests i will pass to review the issue

lucassc’s picture

lucassc
he/him
Primary language Portuguese, Brazil
Location Rio de Janeiro
commented
Assigned: Unassigned » lucassc

Hi! I'll review.

lucassc’s picture

lucassc
he/him
Primary language Portuguese, Brazil
Location Rio de Janeiro
commented
Assigned: lucassc » Unassigned
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

I applied the patch in #20 for branch 7.x-4.x and works well, tests are passing and there's no coding standards problem introduced by the change anymore.

liam morland’s picture

liam morland
Primary language English
Location Ontario, CA 🇨🇦
commented
Status: Reviewed & tested by the community » Needs work

Please put the latest patch into a merge request.

lucassc’s picture

lucassc
he/him
Primary language Portuguese, Brazil
Location Rio de Janeiro
commented
Status: Needs work » Needs review

It's up to date now, I just rerolled MR !167's branch to sync it up with 7.x-4.x.

lucassc’s picture

lucassc
he/him
Primary language Portuguese, Brazil
Location Rio de Janeiro
commented

Tests are still failing, but as they passed in #20 (as a patch) I believe it may be related to what was said in #17:

Sometimes tests on merge requests falsely fail. Instead, a patch needs to be uploaded. This is reported in #3194156: Patches and Merge Requests lead to different test results.

lucassc’s picture

lucassc
he/him
Primary language Portuguese, Brazil
Location Rio de Janeiro
commented
StatusFileSize
new 3016408-27.patch760 bytes

Uploading a patch from MR's plain diff to double check if it's a false fail.

vitorbs’s picture

vitorbs commented
Assigned: Unassigned » vitorbs

I'll do the review.

liam morland’s picture

liam morland
Primary language English
Location Ontario, CA 🇨🇦
commented

The comment being removed says this:

- // @todo HTML5 browsers apparently do not include the max value if it does
- // not line up with step. Restore this if needed in the future.

What is the current behaviour in browsers? Does it work with this patch in a consistent way?

vitorbs’s picture

vitorbs commented
Assigned: vitorbs » Unassigned
Status: Needs review » Reviewed & tested by the community

As the tests passed for the patch I think we can consider it a false fail.

If no one else disagrees I think this can be marked as RTBC.

liam morland’s picture

liam morland
Primary language English
Location Ontario, CA 🇨🇦
commented
Status: Reviewed & tested by the community » Fixed

Thanks everyone!

Status: Fixed » Closed (fixed)

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