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

Adding validatejs to validate forms #1005

Merged

Conversation

damianpm
Copy link
Contributor

@damianpm damianpm commented Jul 16, 2019

What this PR does / why we need it:

Adds form validation to PF4/React forms using Validate.js

Which issue(s) this PR fixes

https://issues.jboss.org/browse/THREESCALE-2993

Special notes for your reviewer:

Pros and cons of Validate.js

Pros:

  • Small (5.05KB, minified and gzipped)
  • Non-intrusive (It doesn't force any styling)
  • It may validate a full form or single fields
  • Active project and community
  • MIT License

Cons:

  • Depends on jQuery (but we already have jQuery, so maybe not a con)

validatejs

Demo:

form-validation

@damianpm damianpm force-pushed the THREESCALE-2993-Client-Side-Form-Validation-ValidateJS branch 3 times, most recently from 949ed36 to 36671e3 Compare July 17, 2019 14:05
@codecov
Copy link

codecov bot commented Jul 18, 2019

Codecov Report

Merging #1005 into master will decrease coverage by 0.04%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1005      +/-   ##
==========================================
- Coverage   93.03%   92.98%   -0.05%     
==========================================
  Files        2449     2447       -2     
  Lines       80830    80662     -168     
==========================================
- Hits        75197    75001     -196     
- Misses       5633     5661      +28
Impacted Files Coverage Δ
.../initializers/backport_pg_10_support_to_rails_4.rb 30% <0%> (-50%) ⬇️
config/initializers/postgres.rb 50% <0%> (-50%) ⬇️
config/initializers/access_token_authentication.rb 55.55% <0%> (-22.23%) ⬇️
app/models/application_record.rb 62.5% <0%> (-15.63%) ⬇️
test/unit/array_test.rb 85.71% <0%> (-14.29%) ⬇️
app/queries/new_accounts_query.rb 71.73% <0%> (-10.87%) ⬇️
lib/tasks/impersonation_admin_user.rake 90% <0%> (-10%) ⬇️
app/lib/three_scale/api/responder.rb 95.74% <0%> (-2.13%) ⬇️
spec/acceptance/api/user_spec.rb
spec/acceptance/api/cms_template_spec.rb

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9621006...5d90550. Read the comment docs.

@damianpm damianpm force-pushed the THREESCALE-2993-Client-Side-Form-Validation-ValidateJS branch 3 times, most recently from 0ebbc3e to c32c9e3 Compare July 18, 2019 11:02
@damianpm damianpm marked this pull request as ready for review July 18, 2019 11:06
@damianpm damianpm force-pushed the THREESCALE-2993-Client-Side-Form-Validation-ValidateJS branch from c32c9e3 to 672c95b Compare July 18, 2019 11:08
thomasmaas
thomasmaas previously approved these changes Jul 18, 2019
Copy link
Member

@thomasmaas thomasmaas left a comment

Choose a reason for hiding this comment

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

Looks ok to me but would be good if @didierofrivia also had a look

Copy link
Contributor

@josemigallas josemigallas left a comment

Choose a reason for hiding this comment

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

validateAllFields is redundant, we should validate each field on change so that we can disable/enable the submit button depending on form's validity.

About the specs: they are not correct, handleChange is being mocked which is wrong (I left a comment about it). Also, there is no trace of validation in the specs although this PR is adding that. I think the following is missing:

  • Add wrong value to a field -> check for "error" style and also error in state
  • Add valid value to a field -> check for no "error" style and no error in state
  • Add any wrong value to the form -> button should disable, and the other way around
  • Check different wrong values such as empty, too short, different passwords, etc

Copy link
Member

@didierofrivia didierofrivia left a comment

Choose a reason for hiding this comment

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

I've suggested just a change since I wanted to avoid duplicity since the changes suggested by @josemigallas are valid, specially the ones about the specs.

@damianpm damianpm force-pushed the THREESCALE-2993-Client-Side-Form-Validation-ValidateJS branch from 672c95b to 55ef407 Compare July 19, 2019 16:09
@damianpm damianpm force-pushed the THREESCALE-2993-Client-Side-Form-Validation-ValidateJS branch 2 times, most recently from 8a84565 to c321760 Compare August 30, 2019 15:52
@damianpm damianpm force-pushed the THREESCALE-2993-Client-Side-Form-Validation-ValidateJS branch 2 times, most recently from 5d90550 to 1626bae Compare September 2, 2019 17:35
@damianpm damianpm force-pushed the THREESCALE-2993-Client-Side-Form-Validation-ValidateJS branch from 1626bae to 02799ec Compare September 2, 2019 18:09
Copy link
Contributor

@josemigallas josemigallas left a comment

Choose a reason for hiding this comment

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

Tentatively approved

@damianpm damianpm merged commit 5527736 into master Sep 3, 2019
@hallelujah hallelujah deleted the THREESCALE-2993-Client-Side-Form-Validation-ValidateJS branch February 28, 2020 11:10
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.

5 participants