Add Grunion Editor View. by georgestephanis · Pull Request #7490 · Automattic/jetpack

georgestephanis · 2017-07-19T21:18:49Z

Fixes #7063
Fixes #1849
Fixes #1055
Fixes #642
Fixes #604
Fixes #254

This is a redo of #6685 because I seem to have made that ugly by rebasing it. :\

The diff to implement this on wpcom is at D6457

This is currently just adding the overrides -- I'm intentionally not removing the old legacy code yet, to ease the transition on any users that prefer the old method. They can just disable the new include line, and the old way will be back again.

Props @MichaelArestad @azaozz and @georgestephanis for building this at the 2016 a8c GM.

Changes proposed in this Pull Request:

Add a visual interface for Contact Forms.

Testing instructions:

Add a contact form to a post.
Edit an existing contact form in a post.
Switch to the html editor. Back again!
Add a second or third contact form to a post.
Open multiple forms to editing mode at the same time.

…tomattic/jetpack into contact-form/visual-shortcode

georgestephanis · 2017-07-19T21:19:56Z

Our travis build for 5.2/5.3 kept failing repeatedly, so thinking it was something that may have been fixed in master, I rebased and then made this repeat of the pr.

kraftbj · 2017-07-19T21:21:37Z

modules/contact-form.php


 include dirname( __FILE__ ) . '/contact-form/grunion-contact-form.php';
+
+if ( is_admin() ) {


What about a filter here to disable the new feature. I would specifically note in the inline docs (and maybe in the filter name) that it is a temporary filter. Maybe say we'll keep it in for the next three? six? months or unless there is a security issue in the older code that we opt to remove it before that time, etc etc etc.

Care to add a commit to introduce it? Maybe change the line to

if ( is_admin() && apply_filters( 'tmp_grunion_allow_editor_view', true ) ) {

For a gentle way to use the older UI to give site owners time to update tutorials, instruct clients, etc. Noting a timeline to remove in six months.

kraftbj · 2017-07-19T22:27:58Z

5.2/5.3 is likely due to #7453 . I'm going to merge that one and see if that resolves it. (Merging out of process only since it is a unit test issue, not production code).

ntwb · 2017-07-19T22:32:55Z

Indeed that is the cause, the 5.2 job is currently running on Trusty, Trusty doesn't have PHP 5.2 or 5.3 available, updating master after #7453 should get this sorted

georgestephanis · 2017-07-20T14:26:11Z

Cherry picked them over.

georgestephanis · 2017-07-20T14:29:17Z

Updated D6457 to reflect new filter and filter docs. Thanks @kraftbj!

georgestephanis · 2017-07-20T14:34:44Z

As we are moving the contact form button into the TinyMCE bar, should we do some sort of tooltip callout pointing out the change to folks? Or maybe just keep the old button as well? It is possible to gloss over.

If this merges, I'll do a second PR to add it back in if we want to keep it.

georgestephanis · 2017-07-20T14:56:28Z

I'm doing some last minute RTL testing before we merge this. It's totally functional, but some stuff isn't being mirrored.

This will handle swapping the sub-iframe to match rtl when needed.

georgestephanis · 2017-07-20T15:40:59Z

As of this point, the phab diff is still in sync with the PR.

MichaelArestad · 2017-07-20T15:41:25Z

Taking it for a spin.

* Changed edit mode card focus and hover styles * Changed edit mode background color to grey to better indicate it needs to be saved/exited * Fixed Required toggle positioning via flex * Fixed label spacing * Fixed primary button styling in edit mode * Removed a bit of unused styles

MichaelArestad · 2017-07-20T17:12:15Z

Pushed several style changes.

Still planning on trying:

implementing sticky for buttons
updating preview section headers to be pretty
separating button groups in edit mode (move cancel/update to the right side)
more spacing tweaks
removing the pesky space at the bottom of the box in edit-mode

georgestephanis · 2017-07-20T17:25:00Z

With the changes in 3cfd8ff implementing flex-grow and such in lieu of floating, this will be bad in IE9 and below. Which is probably fine, just wanted to make that explicit.

georgestephanis · 2017-07-20T18:49:48Z

The phabricator diff -- D6457 -- is now up to date with ~.

zinigor

Code-wise everything seems to be OK except for several i18n nitpicks. Plus, the JS could use a style pass - there are spaces missing here and there.
I'm going to do a testing run next.

zinigor · 2017-07-20T20:40:08Z

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

+			'checkbox'          => __( 'Checkbox', 'jetpack' ),
+			'checkbox-multiple' => __( 'Checkbox with Multiple Items', 'jetpack' ),
+			'select'            => __( 'Drop down', 'jetpack' ),
+			'radio'             => __( 'Radio', 'jetpack' ),


Every one of these translatable strings could really use context. Some of them need it badly, like Drop down can be translated at least three different ways without knowing what it means. If we already have strings like these, I would propose just adding translator comments to avoid creating new strings:

/* translators: label name to choose a dropdown field for a contact form */ 'select' => __( 'Drop down', 'jetpack' ),

Also, shouldn't it be Dropdown?

This was just pulling across via

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

Lines 185 to 193 in 764a699

<option value="checkbox"><?php esc_html_e( 'Checkbox', 'jetpack' ); ?></option>

<option value="checkbox-multiple"><?php esc_html_e( 'Checkbox with Multiple Items', 'jetpack' ); ?></option>

<option value="select"><?php esc_html_e( 'Drop down', 'jetpack' ); ?></option>

<option value="email"><?php esc_html_e( 'Email', 'jetpack' ); ?></option>

<option value="name"><?php esc_html_e( 'Name', 'jetpack' ); ?></option>

<option value="radio"><?php esc_html_e( 'Radio', 'jetpack' ); ?></option>

<option value="text" selected="selected"><?php esc_html_e( 'Text', 'jetpack' ); ?></option>

<option value="textarea"><?php esc_html_e( 'Textarea', 'jetpack' ); ?></option>

<option value="url"><?php esc_html_e( 'Website', 'jetpack' ); ?></option>

zinigor · 2017-07-20T20:41:42Z

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

+
+			<label class="grunion-required">
+				<input type="checkbox" name="required" value="1" <# if ( data.required ) print( 'checked="checked"' ) #> />
+				<span><?php esc_html_e( 'Required?', 'jetpack' ); ?></span>


Same here, context needed.

georgestephanis · 2017-07-21T15:18:05Z

Just following up to ^^, heard from @zinigor in Slack to just leave the translator notes be as it's just pulling existing strings across.

@zinigor

heard from @zinigor in Slack to just leave the translator notes be as it's just pulling existing strings across.

georgestephanis · 2017-07-24T19:42:03Z

modules/contact-form/js/editor-view.js

+				named,
+				fields = '',
+				stylesheet_url = ( 1 === window.isRtl )
+					? grunionEditorView.inline_editing_style_rtl


@zinigor

modules/contact-form/js/editor-view.js line 71 col 21 Bad line breaking before '?'.

Build tools are erroring on this saying it's bad on jshint.

yarn lint

eliorivero · 2017-07-24T21:06:56Z

Works great! Merging.

georgestephanis added 9 commits March 16, 2017 09:40

Add Grunion Editor View.

0f13c58

Add reference phpdoc for filter.

eb78182

Flag TinyMCE as a global.

b119021

Tidying some js lint issues.

ea99905

Add Grunion Editor View.

1f08452

Add reference phpdoc for filter.

f50e3b6

Flag TinyMCE as a global.

28b23cd

Tidying some js lint issues.

2f0bfac

Merge branch 'contact-form/visual-shortcode' of https://github.com/Au…

2094e3a

…tomattic/jetpack into contact-form/visual-shortcode

georgestephanis added [Feature] Forms Touches WP.com Files [Pri] High [Status] Needs Review This PR is ready for review. Enhancement Changes to an existing feature — removing, adding, or changing parts of it labels Jul 19, 2017

georgestephanis added this to the 5.2 milestone Jul 19, 2017

georgestephanis self-assigned this Jul 19, 2017

kraftbj reviewed Jul 19, 2017

View reviewed changes

Adds a temporary filter to disable the editor view

d55331d

For a gentle way to use the older UI to give site owners time to update tutorials, instruct clients, etc. Noting a timeline to remove in six months.

ntwb added 2 commits July 20, 2017 10:25

Travis CI: Use Trusty by default & Precise for PHP 5.2

afc35d8

Add dist: precise for PHP 5.3

7e2549a

georgestephanis added 3 commits July 20, 2017 11:07

Handle RTL generation and enqueue of css.

47ed74d

Typo!

59a2e79

Set LTR explicitly so it gets flipped.

c5982bb

This will handle swapping the sub-iframe to match rtl when needed.

MichaelArestad 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 Jul 20, 2017

MichaelArestad approved these changes Jul 20, 2017

View reviewed changes

zinigor previously requested changes Jul 20, 2017

View reviewed changes

Code style pass for the JS part of the new CF editor view.

b453e8f

georgestephanis commented Jul 24, 2017

View reviewed changes

Fixed lint warning.

2a02e64

georgestephanis requested a review from a team as a code owner July 24, 2017 20:27

eliorivero merged commit cf1a73c into master Jul 24, 2017

eliorivero removed the [Status] Ready to Merge Go ahead, you can push that green button! label Jul 24, 2017

eliorivero deleted the contact-form/visual-shortcode-2 branch July 24, 2017 21:07

georgestephanis mentioned this pull request Jul 25, 2017

Reintroduce button above editor to add contact forms to posts. #7530

Merged

This was referenced Aug 4, 2017

Fix conflict with Jetpack plugin when loading the core/freeform block WordPress/gutenberg#2229

Merged

Multiple errors when loading TinyMCE in other contexts #7598

Closed


		include dirname( __FILE__ ) . '/contact-form/grunion-contact-form.php';

		if ( is_admin() ) {

	<option value="checkbox"><?php esc_html_e( 'Checkbox', 'jetpack' ); ?></option>
	<option value="checkbox-multiple"><?php esc_html_e( 'Checkbox with Multiple Items', 'jetpack' ); ?></option>
	<option value="select"><?php esc_html_e( 'Drop down', 'jetpack' ); ?></option>
	<option value="email"><?php esc_html_e( 'Email', 'jetpack' ); ?></option>
	<option value="name"><?php esc_html_e( 'Name', 'jetpack' ); ?></option>
	<option value="radio"><?php esc_html_e( 'Radio', 'jetpack' ); ?></option>
	<option value="text" selected="selected"><?php esc_html_e( 'Text', 'jetpack' ); ?></option>
	<option value="textarea"><?php esc_html_e( 'Textarea', 'jetpack' ); ?></option>
	<option value="url"><?php esc_html_e( 'Website', 'jetpack' ); ?></option>

Conversation

georgestephanis commented Jul 19, 2017

Changes proposed in this Pull Request:

Testing instructions:

Uh oh!

georgestephanis commented Jul 19, 2017

Uh oh!

kraftbj Jul 19, 2017

Choose a reason for hiding this comment

Uh oh!

georgestephanis Jul 19, 2017

Choose a reason for hiding this comment

Uh oh!

kraftbj commented Jul 19, 2017

Uh oh!

ntwb commented Jul 19, 2017

Uh oh!

georgestephanis commented Jul 20, 2017

Uh oh!

georgestephanis commented Jul 20, 2017

Uh oh!

georgestephanis commented Jul 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

georgestephanis commented Jul 20, 2017

Uh oh!

georgestephanis commented Jul 20, 2017

Uh oh!

MichaelArestad commented Jul 20, 2017

Uh oh!

MichaelArestad commented Jul 20, 2017

Uh oh!

georgestephanis commented Jul 20, 2017

Uh oh!

georgestephanis commented Jul 20, 2017

Uh oh!

zinigor left a comment

Choose a reason for hiding this comment

Uh oh!

zinigor Jul 20, 2017

Choose a reason for hiding this comment

Uh oh!

georgestephanis Jul 20, 2017

Choose a reason for hiding this comment

Uh oh!

zinigor Jul 20, 2017

Choose a reason for hiding this comment

Uh oh!

georgestephanis commented Jul 21, 2017

Uh oh!

georgestephanis Jul 24, 2017

Choose a reason for hiding this comment

Uh oh!

georgestephanis Jul 24, 2017

Choose a reason for hiding this comment

Uh oh!

eliorivero commented Jul 24, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

georgestephanis commented Jul 20, 2017 •

edited

Loading