Update/972 freeform block add format selector #1030

BoardJames · 2017-06-06T04:14:56Z

The purpose of this pull request is to update the freeform block to add a format selector.

Currently the result looks like this:

This is a part of the process of recreating the mockups shown here:
#972

jasmussen · 2017-06-06T08:14:29Z

Nice! This is coming together fast, cool!

I noticed outlines don't fade out when you type. This seems to be causing some conflicts, where if you select a line of text with the intention of going to the paragraph dropdown, then they actually fade out, when in fact the behavior should be the reverse:

Type text and outlines fade out.
As soon as you move the mouse, or hold shift or another modifier key, or use the arrow keys, outlines fade back in.

See also #657.

youknowriad · 2017-06-06T08:37:15Z

blocks/library/freeform/format-list.js

+	}
+
+	render() {
+		const { formats = [ { text: '', value: '', textStyle: () => '' } ] } = this.props;


Is the default empty format here required?

Probably not.
I was thinking that there would be a render step when the editor hadn't loaded and there would be no formats. I can probably handle this cleaner though so I will revise it.

youknowriad · 2017-06-06T08:39:06Z

blocks/library/freeform/format-list.js

+					aria-label={ wp.i18n.__( 'Change format' ) }
+				>
+					<div className="formats">
+						{ formats.map( ( { text, value } ) => (


Should we avoid mapping here and display only the active format? instead of hiding the other formats with CSS.

Well they're there so the dropdown stays the size of the largest item.

I am stacking the lines on top of each other while hiding all but the active item. This results in the width of the button matching the largest item which appears to be how it was in the mockup.

I am happy to take suggestions on how to achieve this effect in another way.

youknowriad · 2017-06-06T08:39:31Z

blocks/library/freeform/format-list.js

+								className={ value === selectedValue ? 'active' : null }
+								aria-hidden={ value !== selectedValue }
+							>
+								{ text }<br />


What is the br for?

It allows me to stack the lines of text by setting the line-height to 0. As stated above I'm using them to achieve the effect of the dropdown staying the width of the largest item.

youknowriad · 2017-06-06T08:43:24Z

blocks/library/freeform/freeform-block.js

+				{ text: 'Heading 6', value: 'h6', textStyle: () => '' },
+				{ text: 'Preformatted', value: 'pre', textStyle: () => '' },
+			],
+			handleFormatChange: null,


Is there any value in storing formats and handlFormatChange in the state? These variables don't look like state to me.

They are drawn from the editor but I suppose I could keep them outside of the state. I do need to re-render when I set the formats array (in onInit) but I suppose I could force that instead of updating state.

youknowriad

Thanks for doing this, the toolbar is working great. i left some comments above

BoardJames · 2017-06-07T05:33:58Z

@jasmussen I can't figure out how to make the tool-bar fade out when typing starts even looking at the pull request you linked. Any idea what I'm missing?

jasmussen · 2017-06-07T06:03:12Z

I can't figure out how to make the tool-bar fade out when typing starts even looking at the pull request you linked. Any idea what I'm missing?

@iseulde can you give some pointers? ❤️

ellatrix · 2017-06-07T13:52:12Z

@EphoxJames Normally this should handle it: https://github.com/WordPress/gutenberg/blob/update/972-Freeform-block-add-format-selector/editor/modes/visual-editor/block.js#L282

BoardJames · 2017-06-08T02:01:18Z

@jasmussen After adding another function from editable the freeform block seems to be behaving the same as the text block now. Note that the toolbar only seems to come back on mouse click but that is the same behaviour as the text block for me.

BoardJames · 2017-06-08T02:05:30Z

~~Hmm, merging master broke it.~~ (forgot npm install)

BoardJames · 2017-06-08T02:23:26Z

@youknowriad @jasmussen I think I've addressed the review comments. Any more changes needed or is it good to merge?

jasmussen · 2017-06-08T07:48:52Z

Nice work!

I think we're very close at this point. There are some fade issues unrelated to this PR we still need to address separately ( #958 ).

I think people are going to love this block 🌟

A few things:

Can you give the dropdown a negative 1px left margin and top margin? The alignment of the dropdown to the switcher is off by a little, and it looks like there's a double border at the top
There's a z-index issue with the inserter below
Can you give the preformatted option in the inserter a code font? (There's a code font stack here: https://github.com/WordPress/gutenberg/blob/master/editor/assets/stylesheets/_variables.scss#L43)
Can you make the heading options look like headings? I.e. use the serif font (https://github.com/WordPress/gutenberg/blob/master/editor/assets/stylesheets/_variables.scss#L42), bold, and then add some size differential to them? Doesn't have to be 1:1 identical to the actual heading sizes, the important thing is to convey hierarchy. For example, h1 could be 120% size, h2 115%, h3 110%, h4 105%, h5 100%, h6 95%?

Thanks!

BoardJames · 2017-06-09T04:35:47Z

@jasmussen
I have done the left and top margin tweaks and I have done my best with the z-index problem - I hope I have not introduced more z-index bugs in the process.

I can mostly implement what you request for the styling though the key problem is that I'm pulling all the information to populate the format menu (including the styling) from TinyMCE so in theory it could have a menu item called 'preformatted' that was actually h1.

Here is what I got:

I am not very happy with the result and I would welcome suggestions and mockups. Maybe it could be done with a preview that pops-up next to the menu so it doesn't mess-up the list size?

jasmussen · 2017-06-09T07:20:52Z

I can mostly implement what you request for the styling though the key problem is that I'm pulling all the information to populate the format menu (including the styling) from TinyMCE so in theory it could have a menu item called 'preformatted' that was actually h1.

Can you clarify this a little bit, especially the last bit about an H1?

Visually and functionally this looks pretty good to me. I'd like to see if we can do a few more tweaks to it — the font sizes for the headings don't have to be identical to the ones actually in the document, we can relax those a bit, and preformatted needs to match a font both in the freeform block and the dropdown to the font stack used by the separate preformatted block. Se might also want to change the color of items in this dropdown. But none of those changes need to block this PR from getting into master — I dig what I'm seeing!

I am not very happy with the result and I would welcome suggestions and mockups. Maybe it could be done with a preview that pops-up next to the menu so it doesn't mess-up the list size?

Can you clarify what bugs you the most about this? Is it the font size in the menus?

I do realize the menu we are mimicking here looks pretty big too, and we can certainly do better. But this is how it looks in the current editor:

But let me know what your primary concerns are, and I'll see if I can think up something to make us all happy. Right now I'm pretty happy though, and I think if we can get a signoff from @youknowriad we might just want to merge this in and iterate.

youknowriad · 2017-06-09T10:30:08Z

blocks/library/freeform/format-list.js

+		styleText.split( ';' ).map(
+			( stylePart ) => {
+				const [ cssKey, cssValue ] = stylePart.split( ':', 2 );
+				const reactKey = cssKey.split( '-' ).map(


Could we use lodash's camelCase utility?

Yep that seems to work perfectly. I did not know it existed.

youknowriad · 2017-06-09T10:33:05Z

blocks/library/freeform/format-list.js

+ */
+import './format-list.scss';
+
+function naiveCss2Jsx( styleText ) {


Can we unit test this utility?

Certainly can. I have added unit tests for the format stylings that TinyMCE is outputting.

youknowriad · 2017-06-09T10:37:51Z

blocks/library/freeform/format-list.js

+	render() {
+		const { formats } = this.props;
+		const selectedValue = this.props.value;
+		const noFormat = { text: wp.i18n.__( 'No format' ), value: null };


Could we use the gutenberg wp modules consistently? I mean avoid using the global wp and use import { __ } from 'i18n';?

Ok, I have done that.

jasmussen · 2017-06-09T11:21:08Z

Did text selection break in this branch? I can't seem to select text.

androb · 2017-06-09T17:57:42Z

@jasmussen I'm seeing selection problems too

jasmussen · 2017-06-09T17:58:42Z

To be clear I'm not convinced they are from this branch. I could swear I saw it in master recently.

BoardJames · 2017-06-12T00:34:33Z

Note that the failing checks are also failing on master - it looks like someone merged a broken test.

BoardJames · 2017-06-12T00:38:11Z

Text selection is currently broken on master and as far as I can tell the problems have nothing to do with this block.

BoardJames · 2017-06-12T03:42:50Z

@jasmussen

Can you clarify this a little bit, especially the last bit about an H1?

I am accessing the format levels from TinyMCE. The TinyMCE editor object has a property called buttons which contains a list of all the buttons that have been registered either by the TinyMCE core or by plugins. Included in that list is the formatselect which contains a list of available TinyMCE formats.
tinyMCE.activeEditor.buttons.formatselect().values
Each of those values has 3 properties:

text - the text representation i.e. "Paragraph"
value - the editor representation i.e. "p"
textStyle - a function that returns the css representing the format i.e. 'font-family:"Noto Serif",serif;font-size:16px;font-weight:400;font-style:normal;text-decoration:none;text-transform:none;color:rgb(68, 68, 68);background-color:transparent;border:;border-radius:;outline:;text-shadow:none;'

At run time I am converting the output from that textStyle attribute into a JSX compatible format

{
  fontFamily: '"Noto Serif",serif',
  fontSize: '16px',
  fontWeight: '400',
  fontStyle: 'normal',
  textDecoration: 'none',
  textTransform: 'none',
  color: 'rgb(68, 68, 68)',
  backgroundColor: 'transparent',
  border: '',
  borderRadius: '',
  outline: '',
  textShadow: 'none',
}

and then removing the color and backgroundColor rules so they don't interfere with the background rendering as otherwise highlighting the menu background on selection doesn't work. I could retain the colour rules if I used indicators next to the text instead of behind it but that wouldn't match up with other select-lists in gutenberg.

I'd like to see if we can do a few more tweaks to it — the font sizes for the headings don't have to be identical to the ones actually in the document, we can relax those a bit

I would like to write the code that displays the formats so it will automatically adjust to any formats (as opposed to hard-coding styles for each "original" format). I could scale the sizes by finding the largest and smallest and then scaling everything so it fits within a range and everything is proportional. Would that work?

preformatted needs to match a font both in the freeform block and the dropdown to the font stack used by the separate preformatted block.

The stylesheet for the freeform block is probably wrong currently - I will look into it.

[W]e might also want to change the color of items in this dropdown.

Do you mean you want the colours of the items to match the styling on the page (in which case I'll have to change how selection is represented) or that you want different selection colours like in the old version you provided the screenshot from?

Can you clarify what bugs you the most about this? Is it the font size in the menus?

The vertical height used by it annoys me the most so scaling the font sizes might address it. The other thing that annoys me is that the drop-down list is now wider than the menu. The only alternative I could think of is keeping the menu options the normal size but rending a preview on hover next to the option.

BoardJames · 2017-06-12T04:22:43Z

I realized I was missing styling from the heading block, preformatted block and editable.

BoardJames · 2017-06-12T05:00:07Z

I tweaked the line height so there wasn't so much whitespace around the larger items.

BoardJames · 2017-06-12T23:55:33Z

@jasmussen

Right now I'm pretty happy though, and I think if we can get a signoff from @youknowriad we might just want to merge this in and iterate.

Since I haven't heard back from you and I think everything is correct I'll merge it and then it can be iterated on.

BoardJames · 2017-06-14T00:57:52Z

The text selection issue seems to have been fixed on editable by #1104 so I have put in a pull request to fix it for the freeform block #1163 .

tiny-james added 3 commits June 5, 2017 17:01

Prototyping the format selector

310cf90

Merge branch 'master' into update/972-Freeform-block-add-format-selector

2f147d7

Finished the format selector

202ca7d

BoardJames mentioned this pull request Jun 6, 2017

Add block: Freeform #972

Closed

youknowriad reviewed Jun 6, 2017

View reviewed changes

jasmussen mentioned this pull request Jun 6, 2017

Freeform Block: Add paragraph dropdown #1037

Closed

Merge branch 'master' into update/972-Freeform-block-add-format-selector

7b0209f

tiny-james added 2 commits June 7, 2017 14:25

Handle cases where selection does not have a known format

4c4a694

Moved formats and handleFormatChange out of state

eabe788

tiny-james added 2 commits June 8, 2017 11:47

Add onSelectionChange handler from editable to fix toolbar fade

2452ede

Merge branch 'master' into update/972-Freeform-block-add-format-selector

051772c

tiny-james added 3 commits June 9, 2017 10:28

Merge branch 'master' into update/972-Freeform-block-add-format-selector

db8a657

Fixed z-index problems and tweaked margins

6df2860

Use css from TinyMCE to style the format menu (WIP)

134324a

youknowriad reviewed Jun 9, 2017

View reviewed changes

tiny-james added 3 commits June 12, 2017 09:53

Use camelCase from lodash

ce4f093

Import __ from i18n to save repeated typing of wp.i18n.__

fa2cea5

Merge branch 'master' into update/972-Freeform-block-add-format-selector

917c850

tiny-james added 2 commits June 12, 2017 12:09

Fixed naiveCss2Jsx to remove empty style rules

227ed80

Added unit tests for naiveCss2Jsx

1669d87

tiny-james added 3 commits June 12, 2017 15:01

Added missing styling

85327d6

Set line height to match item size

aab0bcf

Merge branch 'master' into update/972-Freeform-block-add-format-selector

badf745

BoardJames merged commit c650280 into master Jun 12, 2017

BoardJames deleted the update/972-Freeform-block-add-format-selector branch June 12, 2017 23:55

Update/972 freeform block add format selector #1030

Update/972 freeform block add format selector #1030

Conversation

BoardJames commented Jun 6, 2017

jasmussen commented Jun 6, 2017

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

Choose a reason for hiding this comment

youknowriad left a comment

Choose a reason for hiding this comment

BoardJames commented Jun 7, 2017

jasmussen commented Jun 7, 2017

ellatrix commented Jun 7, 2017

BoardJames commented Jun 8, 2017

BoardJames commented Jun 8, 2017 • edited Loading

BoardJames commented Jun 8, 2017 • edited Loading

jasmussen commented Jun 8, 2017

BoardJames commented Jun 9, 2017 • edited Loading

jasmussen commented Jun 9, 2017

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

jasmussen commented Jun 9, 2017

androb commented Jun 9, 2017

jasmussen commented Jun 9, 2017

BoardJames commented Jun 12, 2017

BoardJames commented Jun 12, 2017

BoardJames commented Jun 12, 2017

BoardJames commented Jun 12, 2017

BoardJames commented Jun 12, 2017

BoardJames commented Jun 12, 2017

BoardJames commented Jun 14, 2017

BoardJames commented Jun 8, 2017 •

edited

Loading

BoardJames commented Jun 8, 2017 •

edited

Loading

BoardJames commented Jun 9, 2017 •

edited

Loading