Make input fields and "push button" class filterable by jimmyandrade · Pull Request #7614 · Automattic/jetpack

jimmyandrade · 2017-08-09T04:50:26Z

Changes proposed in this Pull Request:

Make the class attribute of inputs and buttons filterable, so that developers can add the classes without having to hack the WordPress core or CSS framework.

Proposed changelog entry for your changes:

Now developers can use the grunion_contact_form_submit_button_class and grunion_contact_form_input_classfilters to customize the class attributes of inputs and buttons in Contact Form.

Make the `class` attribute of inputs and buttons filterable, so that developers can add the classes without having to hack the WordPress core or CSS framework.

modules/contact-form/grunion-contact-form.php

dereksmart · 2017-08-18T16:58:25Z

modules/contact-form/grunion-contact-form.php

+			 *
+			 * @param string $class Additional CSS classes for button attribute.
+			 */
+			$submit_button_class = apply_filters( 'grunion_contact_form_submit_button_class', $submit_button_class );


Let's go with jetpack_contact_form_submit_button_class for the name here

dereksmart · 2017-08-18T16:59:58Z

modules/contact-form/grunion-contact-form.php

 		$placeholder       = $this->get_attribute( 'placeholder' );
 		$class             = $this->get_attribute( 'class' );
+
+		/**


Since the block above has already been documented, this can just be

/** This filter is already documented in modules/contact-form/grunion-contact-form.php */

@SInCE

* Add @SInCE 5.3.0 param in code block; * Change `grunion_contact_form_submit_button_class` to `jetpack_contact_form_submit_button_class`;

jimmyandrade · 2017-10-01T16:28:20Z

@dereksmart changed :)

zinigor

Thanks for iterating with us! I have added several comments, please take a look.

zinigor · 2017-10-02T09:35:00Z

modules/contact-form/grunion-contact-form.php

+			 * Filter the contact form submit button class attribute.
+			 *
+			 * @module contact-form
+			 * @since 5.3.0


Some time has passed since this PR has been submitted, so please edit this to be 5.5.0 instead - this will only make it in the shipped code in 5.5.0.

zinigor · 2017-10-02T09:35:23Z

modules/contact-form/grunion-contact-form.php

 		$field_required    = $this->get_attribute( 'required' );
 		$placeholder       = $this->get_attribute( 'placeholder' );
-		$class             = 'date' === $field_type ? 'jp-contact-form-date' : $this->get_attribute( 'class' );
+    $class             = 'date' === $field_type ? 'jp-contact-form-date' : $this->get_attribute( 'class' );


Indentation is off here, please use two tabs.

zinigor · 2017-10-02T09:40:42Z

modules/contact-form/grunion-contact-form.php

-		$class             = 'date' === $field_type ? 'jp-contact-form-date' : $this->get_attribute( 'class' );
+    $class             = 'date' === $field_type ? 'jp-contact-form-date' : $this->get_attribute( 'class' );
+
+		/** This filter is already documented in modules/contact-form/grunion-contact-form.php */


Sorry, but I think you'll need to bring back the doc block here. I think @dereksmart initially thought that it was the same filter, and it's not. Also please use consistent namespacing. Would jetpack_contact_form_input_class work here instead?

zinigor · 2017-10-02T09:41:01Z

modules/contact-form/grunion-contact-form.php

 			$r .= "\t<p class='contact-submit'>\n";
-			$r .= "\t\t<input type='submit' value='" . esc_attr( $form->get_attribute( 'submit_button_text' ) ) . "' class='pushbutton-wide'/>\n";
+
+			$submit_button_class = 'pushbutton-wide';


This is a real nitpicky suggestion, but you could have avoided adding that line by just passing pushbutton-wide as a default to the apply_filters call down there.

* Removed var for `pushbutton-wide`. Now it goes straight on apply_filters; * Changed from 5.3.0 to 5.5.0 on PHPDoc block; * Indentation fix; * Removed unecessary PHPDoc block.

jimmyandrade · 2017-10-02T16:02:22Z

@dereksmart @zinigor changed :)

zinigor · 2017-10-02T18:45:29Z

Thanks for making changes, looks much better now! Only two things that are need attention now are:

the docblock for the second filter, it should be there;
the filter namespacing is inconsistent, please use the same prefix in both of them.

keoshi · 2018-06-05T12:08:54Z

@jimmyandrade Wanted to see if you missed the comment from @zinigor above. Can you please take a quick look and update the PR? Thanks in advance!

jimmyandrade · 2018-07-02T17:51:17Z

@keoshi sorry for the delay. I'm gonna do this 👍 :)

* Adds the docblock for the second filter; * Make filter namespacing consistent, using the same prefix (jetpack_) in both of them.

jetpacl -> jetpack

jimmyandrade · 2018-07-02T17:57:13Z

@keoshi @zinigor done :)

keoshi · 2018-07-03T08:40:02Z

Thank you, @jimmyandrade — pinging @zinigor so he can take another look at your PR.

jeherve

This looks good now. 🚢

Filters were updated to address the issues.

jetpackbot · 2018-09-07T11:51:12Z

	Warnings
⚠️	"Testing instructions" are missing for this PR. Please add some

This is automated check which relies on PULL_REQUEST_TEMPLATE.We encourage you to follow that template as it helps Jetpack maintainers do their job. If you think 'Testing instructions' or 'Proposed changelog entry' are not needed for your PR - please explain why you think so. Thanks for cooperation 🤖

Generated by 🚫 dangerJS

Filters now include a since comment.

zinigor

Thanks, looks good now!

@see

* Readme: add boilerplate for next release, 6.6 * Add 6.5 to the changelog.txt file * Set boilerplate testing list for 6.6 * Readme: update stable tag to 6.5 * Add bullets to 6.5 changelog items * Readme: add link to previous changelogs This will help folks who want to know more about past releases, while keeping the readme.txt short so as to not overwhelm translators and site owners only looking for information about the last release. * Changelog: add information at the top of the changelog file. * Changelog: add #10054 * Changelog: add #10078 * Changelog: add #10079 * Changelog: add #10064 * Changelog: add #10094 * Changelog: add #10096 * Testing list: add more information based on #10087 * Changelog: add #9847 * Changelog: add #10084 * Changelog: add #9918 * Changelog: add #7614 * Changelog: add #10116 * Changelog: add #10108 * Changelog: add #10041 * Changelog: add #10121 * Changelog: add #10134 * Changelog: add #10130 * Changelog: add #10109 * changelog: add #10137 * changelog: add #9952 * changelog: add #10120 * changelog: add #10162 * Changelog: add #10163 * Changelog: add #10092 * changelog: add #10156 * Changelog: add #10154 * changelog: add #10122 * Changelog: add #10101 * changelog: add #10105 * changelog: add #10190 * Changelog: add #10196 * changelog: add #10152 * Changelog: add #10153 * Testing list: add more details to Site Verification testing steps. @see #10143 (comment) * changelog: add #10194 * Changelog: add #10193

Fixes #7613

9b8ce6f

Make the `class` attribute of inputs and buttons filterable, so that developers can add the classes without having to hack the WordPress core or CSS framework.

jimmyandrade requested a review from georgestephanis as a code owner August 9, 2017 04:50

dereksmart previously requested changes Aug 18, 2017

View reviewed changes

dereksmart added Enhancement Changes to an existing feature — removing, adding, or changing parts of it [Feature] Forms [Status] Needs Review This PR is ready for review. labels Aug 18, 2017

jeherve added [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. and removed [Status] Needs Review This PR is ready for review. labels Sep 7, 2017

Jimmy Andrade added 2 commits October 1, 2017 13:26

PHPDoc adjustments

11dfa31

* Add @SInCE 5.3.0 param in code block; * Change `grunion_contact_form_submit_button_class` to `jetpack_contact_form_submit_button_class`;

Merge branch 'master' into patch-1

f5ff8b4

zinigor previously requested changes Oct 2, 2017

View reviewed changes

Removed unecessary lines & PHPDoc version change

41c5eb9

* Removed var for `pushbutton-wide`. Now it goes straight on apply_filters; * Changed from 5.3.0 to 5.5.0 on PHPDoc block; * Indentation fix; * Removed unecessary PHPDoc block.

jimmyandrade added 2 commits July 2, 2018 14:55

Code-style changes

a5197cf

* Adds the docblock for the second filter; * Make filter namespacing consistent, using the same prefix (jetpack_) in both of them.

Fixes prefix typo

4bdbd2b

jetpacl -> jetpack

jeherve added [Status] Needs Review This PR is ready for review. and removed [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. labels Sep 4, 2018

Contact Form: update docblock version

e6f6c3b

jeherve approved these changes Sep 7, 2018

View reviewed changes

jeherve added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review This PR is ready for review. labels Sep 7, 2018

zinigor approved these changes Sep 7, 2018

View reviewed changes

jeherve merged commit 2d15c2e into Automattic:master Sep 7, 2018

matticbot added the [Status] Needs Changelog label Sep 7, 2018

ghost removed the [Status] Ready to Merge Go ahead, you can push that green button! label Sep 7, 2018

jeherve added [Status] Has Changelog and removed [Status] Needs Changelog labels Sep 14, 2018

jeherve added a commit that referenced this pull request Sep 24, 2018

Changelog: add #7614

3c2b123

jimmyandrade deleted the patch-1 branch October 19, 2018 18:47

Conversation

jimmyandrade commented Aug 9, 2017

Changes proposed in this Pull Request:

Proposed changelog entry for your changes:

Uh oh!

Uh oh!

dereksmart Aug 18, 2017

Choose a reason for hiding this comment

Uh oh!

dereksmart Aug 18, 2017

Choose a reason for hiding this comment

Uh oh!

jimmyandrade commented Oct 1, 2017

Uh oh!

zinigor left a comment

Choose a reason for hiding this comment

Uh oh!

zinigor Oct 2, 2017

Choose a reason for hiding this comment

Uh oh!

zinigor Oct 2, 2017

Choose a reason for hiding this comment

Uh oh!

zinigor Oct 2, 2017

Choose a reason for hiding this comment

Uh oh!

zinigor Oct 2, 2017

Choose a reason for hiding this comment

Uh oh!

jimmyandrade commented Oct 2, 2017

Uh oh!

zinigor commented Oct 2, 2017

Uh oh!

keoshi commented Jun 5, 2018

Uh oh!

jimmyandrade commented Jul 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jimmyandrade commented Jul 2, 2018

Uh oh!

keoshi commented Jul 3, 2018

Uh oh!

jeherve left a comment

Choose a reason for hiding this comment

Uh oh!

jetpackbot commented Sep 7, 2018

Uh oh!

zinigor left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

jimmyandrade commented Jul 2, 2018 •

edited

Loading