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 required form radio/checkbox fields #1643

Merged
merged 13 commits into from
Sep 3, 2020

Conversation

EvanHerman
Copy link
Contributor

@EvanHerman EvanHerman commented Aug 25, 2020

Note

If using Go to test, this will require godaddy-wordpress/go#579

Description

Made sure that the checkboxes/radio fields of form block, when set to required, prevent the form from being submitted until at least one option/checkbox is selected.

Resolves #1517 and resolves #1653.

Note: HTML does not provide a way to require checkbox groups. The workaround was to enqueue a script and display a notice above the field when no checkboxes were checked.

The notice: Please select at least one checkbox.
Wrapped in the filter: coblocks_form_checkbox_required_text

Screenshots

form-checkboxes

Types of changes

Bug fix (non-breaking change which fixes an issue)

How has this been tested?

Manually

Checklist:

  • My code is tested
  • My code has proper inline documentation
  • I've included any necessary tests
  • I've included developer documentation
  • I've added proper labels to this pull request

@EvanHerman EvanHerman added [Type] Enhancement Something new that adds functionality [Status] Needs Review Tracking pull requests that need another set of eyes labels Aug 25, 2020
@EvanHerman EvanHerman added this to the Next Release milestone Aug 25, 2020
@EvanHerman EvanHerman requested a review from jrtashjian as a code owner August 25, 2020 00:02
@EvanHerman EvanHerman self-assigned this Aug 25, 2020
@cypress
Copy link

cypress bot commented Aug 25, 2020



Test summary

58 10 0 0


Run details

Project CoBlocks
Status Failed
Commit 036a232
Started Sep 3, 2020 7:40 PM
Ended Sep 3, 2020 7:43 PM
Duration 03:29 💡
OS Linux Debian - 10.5
Browser Multiple

View run in Cypress Dashboard ➡️


Failures

Run group: e2e-chrome (Linux, Chrome )
src/extensions/layout-selector/test/layout-selector.cypress.js Failed
1 Extension: Layout Selector > shows modal on add new "page" post_type
2 Extension: Layout Selector > loads layouts of each category
3 Extension: Layout Selector > inserts layout into page
4 Extension: Layout Selector > inserts blank layout into page
5 Extension: Layout Selector > does not open modal when disabled via the "Editor Settings" panel
Run group: e2e-firefox (Linux, Firefox )
src/extensions/layout-selector/test/layout-selector.cypress.js Failed
1 Extension: Layout Selector > shows modal on add new "page" post_type
2 Extension: Layout Selector > loads layouts of each category
3 Extension: Layout Selector > inserts layout into page
4 Extension: Layout Selector > inserts blank layout into page
5 Extension: Layout Selector > does not open modal when disabled via the "Editor Settings" panel

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

@jrtashjian jrtashjian removed their request for review August 31, 2020 15:27
Copy link
Member

@AnthonyLedesma AnthonyLedesma left a comment

Choose a reason for hiding this comment

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

Left a few comments regarding changes.

includes/class-coblocks-form.php Show resolved Hide resolved
src/js/coblocks-checkbox-required.js Outdated Show resolved Hide resolved
includes/class-coblocks-form.php Outdated Show resolved Hide resolved
.dev/tests/phpunit/includes/test-coblocks-form.php Outdated Show resolved Hide resolved
@EvanHerman
Copy link
Contributor Author

@AnthonyLedesma Good for another review 👌

@AnthonyLedesma
Copy link
Member

Confirmed this closes #1653.

@AnthonyLedesma AnthonyLedesma linked an issue Sep 3, 2020 that may be closed by this pull request
4 tasks
Copy link
Member

@AnthonyLedesma AnthonyLedesma left a comment

Choose a reason for hiding this comment

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

We are very close. The required attributes seem to be properly preventing form submission but we have a case where no errors are shown to the user. To replicate this bug use the following markup.

<!-- wp:coblocks/form -->
<!-- wp:coblocks/field-email {"required":true} /-->

<!-- wp:coblocks/field-radio {"required":true,"options":["Yes","No"]} /-->

<!-- wp:coblocks/field-checkbox {"required":true,"options":["Yes","No"]} /-->

<!-- wp:coblocks/field-textarea {"required":true} /-->

<!-- wp:coblocks/field-submit-button {"submitButtonText":"Contact Us"} /-->
<!-- /wp:coblocks/form -->

When you leave the radio options unchecked and fill all other form elements, we have a case where form fails to submit and no errors are shown to the user. There should be an error message here specifying that an option must be selected.

@EvanHerman
Copy link
Contributor Author

@AnthonyLedesma This was a bug with Go, setting the radio buttons to appearance: none;

Feel free to pull down godaddy-wordpress/go#579 and test, or use a different theme.

@AnthonyLedesma AnthonyLedesma self-requested a review September 3, 2020 19:50
Copy link
Member

@AnthonyLedesma AnthonyLedesma left a comment

Choose a reason for hiding this comment

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

Confirmed that the bug reported was with Go.

@AnthonyLedesma AnthonyLedesma merged commit 19426b2 into master Sep 3, 2020
@AnthonyLedesma AnthonyLedesma deleted the fix/required-checkbox-group branch September 3, 2020 19:51
@jrtashjian jrtashjian added [Type] Bug Something that is not working as expected and removed [Status] Needs Review Tracking pull requests that need another set of eyes [Type] Enhancement Something new that adds functionality labels Sep 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Bug Something that is not working as expected
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ISNBAT render a Form which returns errors Required checkbox not working
4 participants