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

Fix for number field validation and behaviour, addition of currency constraint validation #1605

Merged
merged 5 commits into from
Oct 8, 2023

Conversation

t0mgerman
Copy link

Q A
Bug fix? [x]
New feature? [x]
New sample? [ ]
Related issues? fixes #1592 #1604

What's in this Pull Request?

There is some overlap with the PR here - #1601 (comment)
I don't believe my branch, with the changes I've made, has the issue referenced in #1592

This PR makes a few different changes:

Fixes number validation issues

  • In onSubmitClick in DynamicForm.tsx - current code does some validation to determine whether the form should actually save a list item or not. The Number validation added to better support min and max number settings (in this commit 8998b5d) added logic here in an else statement following if (val.isRequired). This means that even if a field is violating it's own min and max constraints - the form is only preventing save when the field isn't required. I have changed this logic to capture constraint violations that occur whether the field is required or not.
  • When a number column is set to "show as percentage" - the numbers entered in Dynamic Form were being multiplied by 100. I believe this is because values saved via the REST API are typically expected to be between 0 and 1 for percentages; so 15 was becoming 1500% in the list. This is not how the OOB list form behaves, so I am doing a conditional division by 100 to counter this effect.
  • The logic was silently failing the form in cases where users left the field blank and the field had a minimum value constraint greater than 0. This is the issue discussed in [DynamicForm] - Number validation is preventing form save in certain circumstances, not enabled for currency fields #1604 -- I have amended the expressions here so that the form does not fail. Existing code earlier in the loop will catch a scenario where the field is blank when also required. The code I have added will check against constraints only when the input is not empty. It will also fail the form in instances where the input is an invalid number (NaN).

Fixes native number input behaviour when using keys / scroll

  • <TextField/> accepts attributes common to HTMLInputElement - which is why type="number" and type="password" among others would work. Pressing the up or down keys or using the mouse wheel while a number field has focus causes the number to go up or down in value. This can be constrained by min and max values if they are provided, but they were not. By pulling them in, we now get correct behaviour - numbers do not go above or below the bounds set in list settings.

Extending of number validation behaviour to Currency fields, and enhancement of error message to format values as currency

  • Number validation has been extended to currency fields, by giving them the same custom error message shared by regular number fields.
  • To display the correct currency symbol in the error message for currency fields, I get the installed LCIDs from SharePoint when DynamicForm loads using PnP/sp. Using the added CurrencyMap.ts file included in this PR, the LCID set on any currency field can be converted to a culture tag like en-GB. If the lookup fails, we use the culture provided in pageContext instead. This can then be used with Intl.NumberFormat to display values correctly - so if a currency field has a maximum value of 50 - the error message will now say "Value must be lower than £50.00". If we were sure available languages in SPO will remain fairly static, the call to regional settings could be replaced with another mapping object (a Record<number,string> object etc).

@michaelmaillot
Copy link
Collaborator

Hi @t0mgerman,

Thanks for this submission!

Following @NishkalankBezawada's merged PR #1601 that covers #1592, would you mind updating your branch to remove conflicts so that I'll have a look at it?

@t0mgerman
Copy link
Author

Hi @michaelmaillot - I'll take a look at this tomorrow.

I've nearly got another branch to publish / PR to submit which refactors parts of DynamicForm, DynamicField and the ControlsTestWebPart - it adds validation expression evaluation and custom formatting (of header, body, footer) to DynamicForm, and I have amended the ControlsTestWebPart so that toggled components and settings persist across reloads. I have a bit of testing to do to make sure I haven't broken anything!

Would it make more sense for me to ensure #1601 is already conflict merged with that new branch, ensure these changes are added and abandon this PR?

@t0mgerman
Copy link
Author

@michaelmaillot I took a look and merged conflicts. Some of the things @NishkalankBezawada was doing in #1601 in DynamicForm I was covering in changes to DynamicField - but functionally they achieve the same thing. Tested merged code on a list with Number column, Number as Percentage, and Currency.

I'll raise a different PR for the work I discussed above, as you may wish to give it more of a review.

@AJIXuMuK AJIXuMuK merged commit 2581b12 into pnp:dev Oct 8, 2023
1 check passed
@AJIXuMuK
Copy link
Collaborator

AJIXuMuK commented Oct 8, 2023

Thanks @t0mgerman for the changes!

@AJIXuMuK AJIXuMuK added this to the 3.16.0 milestone Oct 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants