-
Notifications
You must be signed in to change notification settings - Fork 1
[Block] Add Form templates #171
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
base: develop
Are you sure you want to change the base?
Conversation
export const variations = [ | ||
{ | ||
name: 'email-only-form', | ||
title: __('Quick Signup (Email Only)', 'mailchimp'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, we are using the form header text from the settings, I think we should consider update is based on the form template. what do you think?
Hmm.. not sure honestly. On one hand, it's not likely that the header text will be accurate for all of these different variations and so setting specific ones for each makes sense. But because we have a global setting for this, the user expectation here is probably for that text to be used. I don't think I want to introduce new settings for each variation though.
Because this text can be edited in the block, I think I may be fine with just using the same text for each variation and a user can edit that at the block level if desired. But open to differing opinions here, maybe we use the header text from settings for the default variation but then have hardcoded titles for the rest?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think I want to introduce new settings for each variation though.
Yes, no new settings. We will just use hardcoded header text. So, each variation will be inserted with the harded header text instead of the one from settings, and user can always edit it as per the requirement once block is added.
QA Update: Question
|
Thanks for testing this, @qasumitbagthariya.
Yes, it should change according to the selected template. However, I noticed in the video that the extra fields being shown are marked as required, that's why they are appearing. Thanks for raising this. As per our current setup, required fields are always visible, and we don’t allow hiding them via the block visibility settings (same at the global level). Also, if we don’t pass the required field information while submitting the form, the Mailchimp API responds with a required field error. However, we can bypass the required validation by passing Given that form templates are meant to display only specific fields, we may need to change the current visibility behavior to show only the fields defined in the selected template and skip merge validation accordingly. @dkotter, I’ll make these changes, but just wanted to check if you have a different take on this. Thanks! |
I think that all makes sense and is probably the ideal way to go. Would it be easy enough to show a message somewhere in those scenarios, alerting the user to the fact that the form template they chose doesn't have some of the fields required by Mailchimp and thus merge validation will be turned off? |
Yes, will show the warning message as well for this. Thanks |
@dkotter @qasumitbagthariya This is ready for review now. |
Description of the Change
PR adds form templates to the Mailchimp List Subscribe Form block using the block variations and variation picker. A form template is essentially a form with a predefined set of fields. The PR introduces the following initial form templates:
Note
For the selected template, the form will still include the other form fields in a hidden state, providing flexibility to show or hide additional form fields.
Closes #170
How to test the Change
Changelog Entry
Credits
Props @jeffpaul, @dkotter, @vikrampm1, Romain Deville, @iamdharmesh
Checklist: