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

[WD-18697] feat: Add "other" option with textarea for radio and checkbox inputs in form generator on u.com #14689

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

muhammad-ali-pk
Copy link
Contributor

@muhammad-ali-pk muhammad-ali-pk commented Jan 29, 2025

Done

  • Add support for "other" option with textarea using js.
  • Remove names from checkboxes for fieldSets named "checkbox" or "checkbox-visibility"
  • Drive by: updated form on /aws to include missing inputType for checkbox fieldsets.
  • Drive by: added a missing filter named "slug"

QA

  • Open this demo/iot/smart-city#get-in-touch in your browser.
  • Verify the form opens up.
  • Fill in the form and make sure to select any/all checkboxes/ radios with "Other" option.
  • Submit the form to make sure it works.
  • Monitor the payload from devtools to make sure the names are stripped from checkboxes.
  • Verify form submission in Marketo if possible and have access.

Also do the same as above for demo/embedded#get-in-touch

Issue / Card

Fixes #WD-18697

@webteam-app
Copy link

@muhammad-ali-pk muhammad-ali-pk changed the title [WD-18697] feat: Add "other" option with textarea for radio and checkbox inputs [WD-18697] feat: Add "other" option with textarea for radio and checkbox inputs in form generator on u.com Jan 29, 2025
Copy link

codecov bot commented Jan 29, 2025

Codecov Report

Attention: Patch coverage is 19.04762% with 17 lines in your changes missing coverage. Please review.

Project coverage is 72.00%. Comparing base (c4540a6) to head (142d166).
Report is 22 commits behind head on main.

Files with missing lines Patch % Lines
static/js/src/prepare-form-inputs.js 19.04% 12 Missing and 5 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #14689      +/-   ##
==========================================
- Coverage   72.32%   72.00%   -0.33%     
==========================================
  Files         120      120              
  Lines        3404     3425      +21     
  Branches     1172     1181       +9     
==========================================
+ Hits         2462     2466       +4     
- Misses        917      929      +12     
- Partials       25       30       +5     
Files with missing lines Coverage Δ
static/js/src/prepare-form-inputs.js 55.40% <19.04%> (-14.41%) ⬇️

@petesfrench
Copy link
Contributor

Where can I test the 'other' field textarea?

@muhammad-ali-pk
Copy link
Contributor Author

muhammad-ali-pk commented Jan 29, 2025

@petesfrench There's no currently no form with 'other' option in main branch.
You can test it locally by adding an 'other' option to one of the checkbox fieldsets.

@petesfrench
Copy link
Contributor

@muhammad-ali-pk Ah ok, can you make sure to add that to your QA instructions next time.
And if possible, all QAing should be done from demos as this is the closest to our production environment. It also makes the review process a lot easier for the reviewer

@muhammad-ali-pk
Copy link
Contributor Author

@petesfrench Thanks Pete.

And if possible, all QAing should be done from demos as this is the closest to our production environment. It also makes the review process a lot easier for the reviewer

Yes that's right., however, some use cases like this one can't be QA'ed on demo as it results in a paradox. Like this functionality is needed by forms currently under constructions to work, but has no example at the moment to verify the functionality. What could have been done was to add a migrated form in this PR., however that would have resulted in scope creep for this PR.

@petesfrench
Copy link
Contributor

@muhammad-ali-pk What we usually do is add an example into the PR (you can copy and paste it from one of the forms currently in progress) and then add a note to remove it before merging

@muhammad-ali-pk
Copy link
Contributor Author

@petesfrench Thanks Pete. I have added example forms and updated the QA steps.

@petesfrench
Copy link
Contributor

The value of the 'other' text area is duplicated:
image

@petesfrench
Copy link
Contributor

If you click 'other', type something in the textarea and then deselect it (or select another option for radio buttons), the value of the textarea is still submitted

@petesfrench
Copy link
Contributor

On https://ubuntu-com-14689.demos.haus/internet-of-things/smart-city#get-in-touch, the space between the mobile number input and country is too big due to a rogue <li> element
image

{
"type": "radio",
"id": "other",
"label": "Other",
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
"label": "Other",
"label": "Other - please describe",

As on the live form

"type": "tel",
"id": "phone",
"label": "Mobile/cell phone number"
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing 'country' here, as on the live form

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants