Inserter: Preview saved block on hover #4246

youknowriad · 2018-01-03T13:00:11Z

This PR tries to show a preview of the block to be inserted at the bottom of the inserter menu.
Ideas to improve the design are welcome.

I believe we could introduce a sampleAttributes property for blocks to make this preview more compelling using the sample attributes.

Maybe we could consider dropping the margins between the blocks in the inserter to avoid the flickering effect when moving from a block to another.

mtias · 2018-01-03T13:06:43Z

I believe we could introduce a sampleAttributes property for blocks to make this preview more compelling using the sample attributes.

My thought was an example property, modelled in a way that could be supplied to testing as well.

aduth · 2018-01-03T19:32:11Z

editor/components/inserter/group.js

@@ -56,6 +64,8 @@ export default class InserterGroup extends Component {
 				ref={ bindReferenceNode( block.name ) }
 				tabIndex={ current === block.name || disabled ? null : '-1' }
 				disabled={ disabled }
+				onMouseEnter={ this.hoverBlock( ( block ) ) }


Redundant inner parentheses?

aduth · 2018-01-03T19:33:44Z

editor/components/inserter/group.js

@@ -39,6 +39,14 @@ export default class InserterGroup extends Component {
 		}
 	}

+	hoverBlock( block ) {


The function name doesn't seem entirely accurate. This is not the hover handler itself; it creates a hover handler. Maybe: createToggleBlockHover ?

Also: Maybe it's worthwhile to check this.props.onHover before returning anything here? That way, we could e.g. return null; and avoid creating new function references (i.e. forcing React to reconcile changing function handlers).

aduth · 2018-01-03T19:34:33Z

editor/components/inserter/style.scss

@@ -1,3 +1,7 @@
+$content-height: 250px;


My understanding is these assigned variables are global to all stylesheets, and this seems like it might be a common name. Maybe worth prefixing to act as namespace if we want the variables?

aduth · 2018-01-03T19:34:50Z

blocks/library/code/index.js

+	example: {
+		content: `
+<?php
+	echo "Beep Beeb Boop";


Personally I'd find this reasonable to inline in the sense that even stylistically this is something I'd have on one line:

example: { content: '<?php echo "Beep Beep Boop"; ?>', },

Typo: "Beeb" -> "Beep"

youknowriad · 2018-01-05T11:10:06Z

I added the example API and tried it for the "code" block only.

Can I have some thoughts for this PR? Do you think I should remove the "margin" from the inserter items to avoid the flickering?
What else can I do to polish this?

aduth

The placement below the inserter doesn't work particularly well when the inserter opens downward (i.e. for new posts), where I'm seeing the preview clipping the bottom of the viewport and unable to be scrolled.

aduth · 2018-01-05T19:05:32Z

blocks/library/code/index.js

@@ -32,6 +32,10 @@ registerBlockType( 'core/code', {
 		},
 	},

+	example: {
+		content: '<?php echo "Beep Beep Boop";?>',


Very minor: I'd expect a space before the closing ?>

https://make.wordpress.org/core/handbook/best-practices/coding-standards/php/#opening-and-closing-php-tags

Minor: PHP Guidelines recommend single quotes:

If you’re not evaluating anything in the string, use single quotes.

https://make.wordpress.org/core/handbook/best-practices/coding-standards/php/#single-and-double-quotes

aduth · 2018-01-05T19:06:58Z

editor/components/block-preview/index.js

@@ -0,0 +1,39 @@
+/**
+ * External dependencie


Typo: "dependencie" -> "dependencies"

mtias · 2018-01-10T12:19:09Z

@jasmussen @youknowriad I think this might need a few more iterations, what do you think of enabling only for "saved blocks" initially? That provides immediate value and requires less consideration of dummy data since saved blocks, by their nature, don't need it.

jasmussen · 2018-01-10T12:35:01Z

I think it's fine to start small and test. 👍 👍

youknowriad · 2018-01-10T14:29:06Z

Should I remove the "example" API for now?

mtias · 2018-01-10T19:28:35Z

Should I remove the "example" API for now?

I don't mind retaining it but not documented.

youknowriad · 2018-01-12T09:40:33Z

I updated this, it's now limited to the reusable blocks and I also updated the design of the inserter "blocks" by removing the margin in favor of a padding to avoid the flickering when we move the mouse between saved blocks.

youknowriad · 2018-01-15T09:11:56Z

Can I have a final review here?

jasmussen · 2018-01-15T09:31:24Z

I think visually this looks pretty good. But I'm getting a few errors on hover:

youknowriad · 2018-01-15T12:37:42Z

@jasmussen I'm not able to reproduce the error. Do you know what kind of reusable block produces this error?

jasmussen · 2018-01-15T12:40:50Z

I just left the office, I may not be able to reproduce at home, but it's an old installation with lots of reusable blocks created across different versions. I'm the same session I noticed a reusable block in the recent tab that was actually deleted.

mtias · 2018-01-15T14:24:42Z

I was reliably reproducing this as well.

aduth · 2018-01-16T17:09:00Z

Was the issue I noted in #4246 (review) addressed ?

The placement below the inserter doesn't work particularly well when the inserter opens downward (i.e. for new posts), where I'm seeing the preview clipping the bottom of the viewport and unable to be scrolled.

youknowriad · 2018-01-19T08:23:03Z

@aduth The issue was not handled but not sure what can we do about it? Any ideas? Should we show the preview on the right of the inserter instead? cc @jasmussen

jasmussen · 2018-01-19T08:29:06Z

The effect of the preview when the inserter opens upwards is a blinking effect where you hover over a saved block and the preview kicks in moving the hovered block upwards, and then it goes for a loop.

Can we just show the preview above the inserter, instead of below it, when the inserter opens upwards?

youknowriad · 2018-01-19T08:31:08Z

@jasmussen That's another issue, we probably can do what you suggest but it doesn't solve the "scrolling" issue if there's not enough space.

jasmussen · 2018-01-19T08:33:25Z

Ah, understood.

How about we solve this in two phases. For now, we decide that this is a desktop only enhancement, with no preview on mobile — hover doesn't work too well there anyway. And given that, we show the preview on the right instead of above or below. We can also make it smaller.

youknowriad · 2018-01-19T10:00:32Z

Ok, I moved the preview to the right, I think it's better.

mtias · 2018-01-26T00:31:41Z

I've been able to break it by creating a paragraph block, saving as reusable, selecting it on the editor canvas, then opening the inserter and hovering over it. The editor crashes with:

TypeError: Cannot read property 'getBoundingClientRect' of null at Editable.getFocusPosition (http://localhost/wp-content/plugins/gutenberg/blocks/build/index.js?ver=1516926147:3023:38)

Should be easy to reproduce.

youknowriad · 2018-01-30T08:04:36Z

Good catch @mtias this commit e977dc4 should fix the issue. We were hardcoding selectors that do not exist in all contexts.

youknowriad · 2018-01-31T12:44:27Z

This should be ready for a final review.

jasmussen · 2018-01-31T13:00:34Z

I dig it!

👍 👍 from me, visually

mtias · 2018-01-31T13:02:09Z

@jasmussen I was seeing a few inconsistencies in the visual display of fonts, etc, that I think we should address. Might be worth applying default styles to a generic .wp-block selector and making sure we use that on the preview panel.

jasmussen · 2018-01-31T13:16:23Z

On which previewed blocks did you see these issues?

jasmussen · 2018-02-01T08:38:07Z

I see what you mean:

I pushed a little polish:

Not necessary for this PR, or perhaps at all, but this seems like an ideal use case for shadow dom.

youknowriad · 2018-02-01T08:42:34Z

@jasmussen Unfortunately, we can't use shadow dom because it doesn't work on Firefox and can't be polyfilled properly.

jasmussen · 2018-02-01T08:48:21Z

Unfortunately, we can't use shadow dom because it doesn't work on Firefox and can't be polyfilled properly.

(ノಠ益ಠ)ノ彡┻━┻

No worries, it was mostly a case of "it would be nice" :)

gziolo · 2018-02-01T08:50:11Z

It will look amazing as soon as we add example property to all blocks 🥇

…modal generic

youknowriad · 2018-02-07T13:34:22Z

I'm thinking we can merge this right?

mtias · 2018-02-07T13:37:17Z

@jasmussen we can use a class like .wp-block to assign the fonts, default sizes, etc, in the first place. Instead of relying on the general cascade.

youknowriad · 2018-02-07T13:58:09Z

I wonder if .wp-block is not the generated className of the reusable block

youknowriad added the Needs Design Feedback Needs general design feedback. label Jan 3, 2018

youknowriad self-assigned this Jan 3, 2018

youknowriad requested review from mtias and jasmussen January 3, 2018 13:00

aduth reviewed Jan 3, 2018

View reviewed changes

youknowriad force-pushed the try/block-preview branch 2 times, most recently from 3919ca3 to f444541 Compare January 4, 2018 09:11

aduth reviewed Jan 5, 2018

View reviewed changes

gziolo mentioned this pull request Jan 11, 2018

Blocks: Add regression check for block edit using snapshot testing #4399

Merged

3 tasks

youknowriad force-pushed the try/block-preview branch from 395053c to 24e0dfa Compare January 12, 2018 11:52

youknowriad force-pushed the try/block-preview branch 2 times, most recently from 2f68d7e to 67b89c8 Compare January 19, 2018 13:57

youknowriad force-pushed the try/block-preview branch from 67b89c8 to e977dc4 Compare January 30, 2018 08:03

youknowriad added 4 commits February 1, 2018 12:48

Inserter: Preview block on hover

fb26edf

Block API: Adding the "example" block property

e714fa1

Block Preview: Beep Beep Boop and styling fixes

4bb5ada

Show preview on the right of the inserter

411f7d2

youknowriad force-pushed the try/block-preview branch from 1daee71 to 411f7d2 Compare February 1, 2018 11:48

youknowriad and others added 2 commits February 1, 2018 12:51

Format Toolbar: Make the function computing the position of the link …

e3b769d

…modal generic

Polish blocks a bit.

f80e657

mtias approved these changes Feb 7, 2018

View reviewed changes

youknowriad merged commit d744a97 into master Feb 7, 2018

youknowriad deleted the try/block-preview branch February 7, 2018 13:57

youknowriad changed the title ~~Inserter: Preview block on hover~~ Inserter: Preview saved block on hover Feb 7, 2018

afercia mentioned this pull request Feb 18, 2018

Saved blocks preview: consider to make it available to keyboard users #5121

Closed

Inserter: Preview saved block on hover #4246

Inserter: Preview saved block on hover #4246

Conversation

youknowriad commented Jan 3, 2018

mtias commented Jan 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youknowriad commented Jan 5, 2018

aduth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtias commented Jan 10, 2018

jasmussen commented Jan 10, 2018

youknowriad commented Jan 10, 2018

mtias commented Jan 10, 2018

youknowriad commented Jan 12, 2018 • edited Loading

youknowriad commented Jan 15, 2018

jasmussen commented Jan 15, 2018

youknowriad commented Jan 15, 2018

jasmussen commented Jan 15, 2018

mtias commented Jan 15, 2018

aduth commented Jan 16, 2018

youknowriad commented Jan 19, 2018

jasmussen commented Jan 19, 2018

youknowriad commented Jan 19, 2018

jasmussen commented Jan 19, 2018

youknowriad commented Jan 19, 2018

mtias commented Jan 26, 2018

youknowriad commented Jan 30, 2018

youknowriad commented Jan 31, 2018

jasmussen commented Jan 31, 2018

mtias commented Jan 31, 2018

jasmussen commented Jan 31, 2018

jasmussen commented Feb 1, 2018

youknowriad commented Feb 1, 2018

jasmussen commented Feb 1, 2018

gziolo commented Feb 1, 2018

youknowriad commented Feb 7, 2018

mtias commented Feb 7, 2018

youknowriad commented Feb 7, 2018

youknowriad commented Jan 12, 2018 •

edited

Loading