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.

Sign up for GitHub

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

Jump to bottom

[preferences]: Input field validation for numerical inputs #8264

Merged
merged 1 commit into from
Aug 11, 2020
Merged

[preferences]: Input field validation for numerical inputs #8264

merged 1 commit into from
Aug 11, 2020

Conversation

Anasshahidd21
Copy link
Contributor

@Anasshahidd21 Anasshahidd21 commented Jul 29, 2020 •
edited
Loading

What it does

Fixes: #7741

  • Added a numerical input field validation in the preference widget.
  • Updated the input field type to be number instead of text for numerical fields.
1 2 3

How to test

# Test Case Example Result
1 Input a value below the min range defined by the preference Input -2 in Font Size preference Should show an error message regarding the range
2 Leave the preference value empty Remove an input in Font Size preference Should show an error message regarding input being a number
3 Input a value above the max range defined by the preference Input 151 in Font Size preference Should show an error message regarding the range
4 Input a value that is not an integer Input 1.5 in Line Height preference Should show an error message regarding the type being an integer
5 Make sure an error is generated, focus out of the input - Should get rid of the error and reset the value to last set value
6 Make sure an error is generated, reset the setting - Should reset the setting input to default, and also get rid of the error
7 Generate multiple errors together Input 1111111111111.12 in Line Height preference Should concat different validation error messages

Signed-off-by: Anas Shahid muhammad.shahid@ericsson.com

Review checklist

Reminder for reviewers

@Anasshahidd21 Anasshahidd21 force-pushed the preferenceInputValidator branch 6 times, most recently from c14bab0 to 9752b36 Compare July 29, 2020 18:09
@Anasshahidd21 Anasshahidd21 force-pushed the preferenceInputValidator branch from 9752b36 to c8bc819 Compare July 29, 2020 21:30
@vince-fugnitto vince-fugnitto added the preferences issues related to preferences label Jul 29, 2020
@vince-fugnitto
Copy link
Member

@colin-grant-work would you like to review the pull-request?

@colin-grant-work
Copy link
Contributor

Sure, I'll take a look this morning

@colin-grant-work
Copy link
Contributor

The functionality looks great. I ran it through the tests described and behavior was as advertised. A couple of minor comments, but otherwise it's just about there.

I wonder if it might be possible to abstract the behavior in such a way as to make it usable with other input types? I don't think that has to be done for this PR, though - there's a fair bit of repetition across those components that a good refactor with some suitable higher-order component could address separately.

@Anasshahidd21
Copy link
Contributor Author

Anasshahidd21 commented Jul 30, 2020 •
edited
Loading

I wonder if it might be possible to abstract the behavior in such a way as to make it usable with other input types?

Good point @colin-grant-work, currently as I have noticed we don't support a lot of properties such as minLength and maxLength i.e. part of string-input validation, so I chose not to implement them.

But adding on to your point, a general validator for all types in the future (when support is provided) should be created in a parent class, the best place in my mind could be single-preference-wrapper.tsx which essentially renders the input fields.

@Anasshahidd21 Anasshahidd21 force-pushed the preferenceInputValidator branch 5 times, most recently from cf4a879 to 15742b0 Compare July 31, 2020 16:56
[preferences]: Input field validation
fc0a5fa 
Fixes: eclipse-theia#7741

Added an input field validation in the preference widget.
Updated the input field type to be `number` instead of `text` for
numerical fields.

Signed-off-by: Anas Shahid <muhammad.shahid@ericsson.com>
@Anasshahidd21 Anasshahidd21 force-pushed the preferenceInputValidator branch from 15742b0 to fc0a5fa Compare July 31, 2020 16:59
@Anasshahidd21
Copy link
Contributor Author

@vince-fugnitto @colin-grant-work Thank you for the suggestions and reviews above. I have updated the code, and am ready for another round of review

@colin-grant-work
Copy link
Contributor

colin-grant-work commented Jul 31, 2020 •
edited
Loading

The functionality is working great, and I think the code is looking good, as well. 🎉

vince-fugnitto
vince-fugnitto approved these changes Aug 4, 2020
View reviewed changes
Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

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

The changes work well, I'll give others a chance to review if they'd like and merge later on during the week if there are no objections.

@vince-fugnitto vince-fugnitto merged commit 928a3bf into eclipse-theia:master Aug 11, 2020
@vince-fugnitto vince-fugnitto deleted the preferenceInputValidator branch August 11, 2020 20:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@colin-grant-work colin-grant-work colin-grant-work left review comments

@paul-marechal paul-marechal paul-marechal left review comments

@vince-fugnitto vince-fugnitto vince-fugnitto approved these changes

Assignees
No one assigned
Labels
preferences issues related to preferences
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

preferences: provide input field validation
4 participants
@Anasshahidd21 @vince-fugnitto @colin-grant-work @paul-marechal