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

Jump to bottom

feat(Abide): add validator-specific error messages #11698

Merged
merged 1 commit into from Feb 3, 2020
Merged

feat(Abide): add validator-specific error messages #11698

merged 1 commit into from Feb 3, 2020

Conversation

soylent
Copy link

@soylent soylent commented Feb 24, 2019

Description

It would be great to display different error messages depending on failed validation.

Now you can add different error messages for each validator using the [data-form-error-on] attribute:

<input required type="email">
<span class="form-error" data-form-error-on="required">Required</span>
<span class="form-error" data-form-error-on="pattern">Invalid</span>

BREAKING CHANGE: Foundation.Abide#validateText() no longer checks if the input is required. Use the Foundation.Abide#requiredCheck() method for that.

Types of changes

  • Documentation
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (anything that would change an existing functionality)
  • Maintenance (refactor, code cleaning, development tools...)

Checklist

  • I have read and follow the CONTRIBUTING.md document.
  • The pull request title and template are correctly filled.
  • The pull request targets the right branch (develop or develop-v...).
  • My commits are correctly titled and contain all relevant information.
  • I have updated the documentation accordingly to my changes (if relevant).
  • I have added tests to cover my changes (if relevant).

@DanielRuf
Copy link
Contributor

Hi @soylent,

awesome PR which also includes new tests.

We'll check it and give you feedback.

@soylent
Copy link
Author

soylent commented Feb 27, 2019

Thanks, @DanielRuf. Please let me know if I can make this PR better.

@kball
Copy link
Contributor

kball commented Aug 12, 2019

Given the data-attribute API is the most common approach, I think the backwards incompatibility in the JS API is probably fine, but we'll need to note it in release notes.

There is a merge conflict right now - I can address that when merging. @DanielRuf do you see any reason not to merge this for 6.6?

@DanielRuf
Copy link
Contributor

We could also need unit tests but so far it looks good.

DanielRuf
DanielRuf approved these changes Oct 2, 2019
View reviewed changes
js/foundation.abide.js Outdated
@@ -132,6 +133,15 @@ class Abide extends Plugin {

$error = $error.add(this.$element.find(`[data-form-error-for="${id}"]`));

if (failedValidators) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (failedValidators) {
if (!!failedValidators) {

Copy link
Author

Choose a reason for hiding this comment

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

Updated

js/foundation.abide.js
@@ -417,7 +431,7 @@ class Abide extends Plugin {
// A pattern can be passed to this function, or it will be infered from the input's "pattern" attribute, or it's "type" attribute
pattern = (pattern || $el.attr('pattern') || $el.attr('type'));
var inputText = $el.val();
var valid = false;
var valid = true;
Copy link
Contributor

@DanielRuf DanielRuf Oct 2, 2019 •
edited

Choose a reason for hiding this comment

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

This seems to be unrelated.

Copy link
Author

Choose a reason for hiding this comment

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

Please see my other comment

js/foundation.abide.js
@@ -428,13 +442,6 @@ class Abide extends Plugin {
else if (pattern !== $el.attr('type')) {
valid = new RegExp(pattern).test(inputText);
}
else {
Copy link
Contributor

@DanielRuf DanielRuf Oct 2, 2019 •
edited

Choose a reason for hiding this comment

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

This seems to be unrelated.

Copy link
Author

@soylent soylent Oct 7, 2019 •
edited

Choose a reason for hiding this comment

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

The validateText() method was performing two validations:

  1. pattern validation
  2. checked if the input is required

It returns false if either of the two validations fails, but to implement this feature, I need to distinguish between those two cases, so I removed the second validation (the requiredCheck() method should be used instead). Please see:
https://github.com/foundation/foundation-sites/pull/11698/files#diff-37998d120faf57b3e7580109c8e306e2R346-R347

I think this is a reasonable change because:

  1. the method now does only one thing instead of two
  2. the documentation now matches the implementation

test/visual/abide/error-messages.html
@@ -0,0 +1,120 @@
<!doctype html>
Copy link
Contributor

Choose a reason for hiding this comment

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

Awesome, tests =)

@DanielRuf DanielRuf self-requested a review October 2, 2019 23:18
DanielRuf
DanielRuf suggested changes Oct 2, 2019
View reviewed changes
Copy link
Contributor

@DanielRuf DanielRuf left a comment

Choose a reason for hiding this comment

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

See my feedback.

@weekly-digest weekly-digest bot mentioned this pull request Oct 6, 2019
Closed
@soylent soylent force-pushed the feat/validator-error-messages branch from 3034a3f to 4cd93b3 Compare October 7, 2019 00:56
@DanielRuf
Copy link
Contributor

Please resolve the conflicts / rebase.

feat(abide): add validator-specific error messages
5eac681 
Now you can add different error messages for each validator using the
`[data-form-error-on]` attribute.

<input required type="email">
<span class="form-error" data-form-error-on="required">Required</span>
<span class="form-error" data-form-error-on="pattern">Invalid</span>

BREAKING CHANGE: `Foundation.Abide#validateText()` no longer checks if
the input is required. Use the `Foundation.Abide#requiredCheck()` method
for that.

Closes foundation#10799
@soylent soylent force-pushed the feat/validator-error-messages branch from 4cd93b3 to 5eac681 Compare October 8, 2019 04:11
@soylent
Copy link
Author

soylent commented Oct 8, 2019

Please resolve the conflicts / rebase.

Done!

@weekly-digest weekly-digest bot mentioned this pull request Oct 13, 2019
Closed
@weekly-digest weekly-digest bot mentioned this pull request Jan 19, 2020
Closed
@joeworkman joeworkman merged commit 5a41377 into foundation:develop Feb 3, 2020
@weekly-digest weekly-digest bot mentioned this pull request Feb 9, 2020
Closed
@joeworkman
Copy link
Member

This pull request has been mentioned on Foundation Open Source Community. There might be relevant details there:

https://foundation.discourse.group/t/foundation-for-sites-6-6-2/1936/1

This was referenced Apr 22, 2020
Open
Open
Merged
Open
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kball kball kball approved these changes

@DanielRuf DanielRuf DanielRuf approved these changes

Assignees
No one assigned
Labels
Abide
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Abide] Separating out error messages
4 participants
@soylent @DanielRuf @kball @joeworkman