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

Admin\Profiles\Edit.razor: Form Validation For Rows, Length and Order - Adds Rows Resource Data - Issues #3426, #3446 #3447

Merged
merged 5 commits into from
Oct 30, 2023

Conversation

thabaum
Copy link
Contributor

@thabaum thabaum commented Oct 24, 2023

Fixes #3426
Fixes #3446

Updates Admin\Profiles\Edit.razor:

  • Adds form validation for Rows, Length and Order form input to Profiles\Edit.razor component type="number".
  • Adds form validation for Rows min="1".
  • Replaces the ignored maxLength attribute with max attribute for inputs with type attribute as number.
  • Adds Rows resource data to related Profiles\Edit.resx file.

@thabaum thabaum changed the title Admin\Profiles\Edit.razor: Form Validation For Rows, Length and Order - Adds Rows Resource Data - Issue #3426 Admin\Profiles\Edit.razor: Form Validation For Rows, Length and Order - Adds Rows Resource Data - Issues #3426, #3446 Oct 24, 2023
@sbwalker
Copy link
Member

@thabaum I like the usage of min/max to restrict data entry and provide feedback to the user. I believe the criteria is:

Rows - min: 1 max: 10? {some reasonable number of rows for a textarea UI control so that it is usable... and most browsers allow a user to expand the size of the textarea anyways}
Length - min: 0 max: 524288? {the user profile value is backed by an nvarchar(max) so the only limitation is HTML5 which allows a maximum of 524,288 characters in a textarea}
Order - min: 0 max: 99 - technically this field could accommodate negative values however there is no practical reason to do so.

@thabaum
Copy link
Contributor Author

thabaum commented Oct 26, 2023

@sbwalker happy to hear using min/max attributes for these input form elements validation is inline with the Oqtane Framework.

Should the default text language be updated as well to reflect these limits such as: The number of rows for text entry (minimum:1 to maximum:10)

If so how would you think this would be best presented in the help text? I can make this update if needed since these two files are relating to updating these text's. Otherwise it's ready to go with changes applied.

Order text and Rows text would be the only two I would think need updated to reflect the limits. Length is set pretty high number but the max value can be shown as well since 0 is unlimited.

Order: I could set this to -99 to 99, but 0 works too.

We can put -99 to 99 in the event someone wants to push things above fields that are already 0. But like you said it is a bit much with 0 being one of the ways to do this. I would just hate to see someone already using a negative value run into an issue as it could cause a breaking change as you point out it's possible someone could do this.

@sbwalker sbwalker merged commit b5108b9 into oqtane:net8 Oct 30, 2023
1 check passed
@W6HBR
Copy link
Contributor

W6HBR commented Jan 4, 2024

Just realized this was a breaking change for values I have on two different sites for profile field order and field length.

Each site has approximately 50+ profile fields. The prior order value allowed 9999 and now is restricted to 99 (for what reason?). With the prior 9999 value, I separated profile fields into categories that numbered 1001, 1002, 2001, 2002, etc. to allow adding new fields to categories without the need to reorder all fields. 99 is much too restrictive.

Also, why the arbitrary field length of 9999 when it was previously 524288? I don't see the benefit to using a shorter field length when the underlying field is nvarchar(max).

@thabaum
Copy link
Contributor Author

thabaum commented Jan 4, 2024

@W6HBR I am sorry to hear you had a breaking change with this PR as we did not expect anyone using this high of a value here. I can see the purpose of using large numbers to help organize large categories of profile properties.

I can produce the PR to adjust this as needed to resolve this issue for you so this is fine tuned in the upcoming release.

Are you stating the order max attribute should equal 524288 or is 9999 enough to help with numerical organization of the profile properties?

Since you are one of the community members using this profile area in a more advanced way do you have any other suggestions, feedback or recommendations to help make sure this experience work for all cases the community may need covered?

Once I hear what value you feel most to set this to I will create the issue and submit the changes for review so we can go from there with handling this breaking change with the order max value issue you described to help you avoid having to make all of the profile order adjustments.

We can then get a place for a more open discussion with a review. And if this is something you would like to handle that works for me as well as I have no dogs in this fight currently. I am open to a larger number of supported profile order numbers in Oqtane Framework, at the same time 99 would have worked OK for your site with 50+ as it was still double the number of properties if you had known this restriction prior. But if you have 50, someone will want 100... I have a hunch eventually.

@W6HBR
Copy link
Contributor

W6HBR commented Jan 4, 2024

@thabaum
Hi Cody, I went ahead and did a pull request to change the order and row values. You can see the change and description there.

I was wrong about the length value as I was looking at the wrong version in the change history, as I see you did keep the 524288 value. So the only breaking change looks to be order and row.

FYI - even at 50+ profile fields, the limit of 99 causes an issue as the admin may need to renumber a bunch of them to get their fields in the necessary order (when making later additions). With the 4 characters (9999), this allows doing numbers with gaps which greatly helps in adding new fields to existing categories.

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.

3 participants