Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CARDS-2580 - Number question: allow min/max value to be specified but not enforced #1810

Merged
merged 8 commits into from
Oct 15, 2024

Conversation

marta-
Copy link
Contributor

@marta- marta- commented Sep 18, 2024

Includes a test questionnaire Number Min / Max Value Test.

Copy link
Contributor

@sashaandjic sashaandjic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tested

image

Upon save, the form is not marked as Draft and Invalid if min/max are not enforced.

… not enforced

Addressing review comments - use the default message when min/max values are enforced.
@sashaandjic sashaandjic added bug Something isn't working and removed testing... Testing in progress labels Sep 18, 2024
@sashaandjic
Copy link
Contributor

sashaandjic commented Sep 18, 2024

When enforcing min/max is enabled, the error msg shows up:

  • in black fonts if withing the range
  • in red fonts of not within range

image

image

This is due to the existing behavior, when error msg is blank and we display the old message. Can we just ignore the text in the error msg in this case ( when enforcing min/max is enabled)?

@sashaandjic sashaandjic added testing... Testing in progress and removed bug Something isn't working labels Sep 18, 2024
Copy link
Contributor

@sashaandjic sashaandjic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tested

@sashaandjic sashaandjic added tested Passed manual testing, needs code review and removed testing... Testing in progress labels Sep 18, 2024
@marta-
Copy link
Contributor Author

marta- commented Sep 18, 2024

When enforcing min/max is enabled, the error msg shows up:

  • in black fonts if withing the range
  • in red fonts of not within range

The code has been updated in 2bb40f1 to have the exact same behavior as before (no custom message).

@sashaandjic
Copy link
Contributor

sashaandjic commented Sep 18, 2024

image

It would be nice to always display range message below the input line.

image

@marta- marta- added the urgent Needs to be completed and merged asap label Sep 25, 2024
Copy link
Member

@sdumitriu sdumitriu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This behaves slightly different from enforced ranges:

  • In enforced ranges, the text is validated as it is typed, so if minValue=10, typing 2 will trigger the error, typing 20 will remove the error. In optional ranges, the error is triggered only when the input loses focus, but the error is removed while typing if the value becomes valid.
  • In optional ranges, the bottom line of the input also turns red, which I like, we should do that for enforced ranges as well.
  • If the error message is displayed, deleting the whole value doesn't clear the error message, it still appears to be invalid, but in enforced ranges an empty input is considered valid (if minAnswers=0).
  • Is this a regression, or was it always like this? If I "Hide answer instructions", the error message for enforced ranges is never shown.

… not enforced

Addressing review comments - clear the error for soft limits when the value is empty
… not enforced

Addressing review comments - give the input an error state (red border) when the value is outside enforced limits
@marta-
Copy link
Contributor Author

marta- commented Sep 25, 2024

This behaves slightly different from enforced ranges:

  • In enforced ranges, the text is validated as it is typed, so if minValue=10, typing 2 will trigger the error, typing 20 will remove the error. In optional ranges, the error is triggered only when the input loses focus, but the error is removed while typing if the value becomes valid.

I can add the liveValidation property and make the behavior configurable. Given the "soft" nature of this validation, checking as you type didn't make sense at the time.

  • In optional ranges, the bottom line of the input also turns red, which I like, we should do that for enforced ranges as well.

Updated.

  • If the error message is displayed, deleting the whole value doesn't clear the error message, it still appears to be invalid, but in enforced ranges an empty input is considered valid (if minAnswers=0).

Updated.

  • Is this a regression, or was it always like this? If I "Hide answer instructions", the error message for enforced ranges is never shown.

It has always been like this.

sashaandjic

This comment was marked as outdated.

sdumitriu

This comment was marked as resolved.

… not enforced

Addressing review comments: fixed "when opening a new form with no answers, the unenforced answers start as invalid"
@marta- marta- requested a review from sdumitriu October 1, 2024 22:43
sdumitriu

This comment was marked as resolved.

… not enforced

Addressing review comments:
* Bugfix: soft min/max values are enforced for multivalued answers
* Regression: hard min/max values are no longer enforced for multivalued answers
@marta- marta- added Test me! Ready for testing and removed tested Passed manual testing, needs code review labels Oct 9, 2024
… not enforced

Addressing review comments: allow negatives regardless of minValue if not enforced
@marta-
Copy link
Contributor Author

marta- commented Oct 9, 2024

Not as expected when multiple values are allowed: Long question, maxAnswers=0, range for the values. When the range is enforced, pressing Enter still adds an invalid value as one of the answer values, but when the range is not enforced, Enter doesn't accept the value, but Tab or click outside the input will still select it. This is a regression for enforced questions, and it should be the other way around.

Addressed in 2fea07e .

If the minValue is >=0, then the input field doesn't allow entering negative values. I think we should disable the "no-negatives" behavior when the range is not enforced.

Adressed in 806b87f .

Also with multiple answers, each invalid answer gets the out-of-range message appended. I think it's a good choice, just raising it in case it's not what you want.

This is what we want.

Copy link
Contributor

@sashaandjic sashaandjic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tested

@sashaandjic sashaandjic added tested Passed manual testing, needs code review and removed Test me! Ready for testing labels Oct 11, 2024
Copy link
Member

@sdumitriu sdumitriu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This works well enough, on par with dev. Some things noticed while testing:

  • number, single value, enforced range: entering an out-of-range number will save this invalid value when saving the form
  • number, multiple values, enforced range: entering an out-of-range number and leaving the field does not accept the value, and does not save it when saving the form
  • text, multiple values, validation regexp: entering a non-matching value will still accept and save it
  • in view mode out-of-range numerical values aren't displayed as invalid, but non-matching text values do display the invalid-regexp message

For consistency, we should allow out-of-range numerical values to be saved if entered, and we should display the error/warning messages in view mode.

@sdumitriu sdumitriu merged commit 4565b80 into dev Oct 15, 2024
@sdumitriu sdumitriu deleted the CARDS-2580 branch October 15, 2024 20:12
@sashaandjic
Copy link
Contributor

For consistency, we should allow out-of-range numerical values to be saved if entered, and we should display the error/warning messages in view mode.

+1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tested Passed manual testing, needs code review urgent Needs to be completed and merged asap
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants