-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Update/972 freeform block add format selector #1030
Update/972 freeform block add format selector #1030
Conversation
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:
See also #657. |
} | ||
|
||
render() { | ||
const { formats = [ { text: '', value: '', textStyle: () => '' } ] } = this.props; |
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.
Is the default empty format here required?
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.
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.
aria-label={ wp.i18n.__( 'Change format' ) } | ||
> | ||
<div className="formats"> | ||
{ formats.map( ( { text, value } ) => ( |
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.
Should we avoid mapping here and display only the active format? instead of hiding the other formats with CSS.
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.
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.
className={ value === selectedValue ? 'active' : null } | ||
aria-hidden={ value !== selectedValue } | ||
> | ||
{ text }<br /> |
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.
What is the br
for?
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.
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.
{ text: 'Heading 6', value: 'h6', textStyle: () => '' }, | ||
{ text: 'Preformatted', value: 'pre', textStyle: () => '' }, | ||
], | ||
handleFormatChange: null, |
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.
Is there any value in storing formats
and handlFormatChange
in the state? These variables don't look like state to me.
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.
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.
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.
Thanks for doing this, the toolbar is working great. i left some comments above
@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? |
@iseulde can you give some pointers? ❤️ |
@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 |
@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. |
|
@youknowriad @jasmussen I think I've addressed the review comments. Any more changes needed or is it good to merge? |
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:
Thanks! |
@jasmussen 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. 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 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!
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. |
styleText.split( ';' ).map( | ||
( stylePart ) => { | ||
const [ cssKey, cssValue ] = stylePart.split( ':', 2 ); | ||
const reactKey = cssKey.split( '-' ).map( |
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.
Could we use lodash's camelCase
utility?
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.
Yep that seems to work perfectly. I did not know it existed.
*/ | ||
import './format-list.scss'; | ||
|
||
function naiveCss2Jsx( styleText ) { |
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.
Can we unit test this utility?
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.
Certainly can. I have added unit tests for the format stylings that TinyMCE is outputting.
render() { | ||
const { formats } = this.props; | ||
const selectedValue = this.props.value; | ||
const noFormat = { text: wp.i18n.__( 'No format' ), value: null }; |
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.
Could we use the gutenberg wp
modules consistently? I mean avoid using the global wp
and use import { __ } from 'i18n';
?
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.
Ok, I have done that.
Did text selection break in this branch? I can't seem to select text. |
@jasmussen I'm seeing selection problems too |
To be clear I'm not convinced they are from this branch. I could swear I saw it in master recently. |
Note that the failing checks are also failing on master - it looks like someone merged a broken test. |
Text selection is currently broken on master and as far as I can tell the problems have nothing to do with this block. |
I am accessing the format levels from TinyMCE. The TinyMCE editor object has a property called
At run time I am converting the output from that textStyle attribute into a JSX compatible format
and then removing the
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?
The stylesheet for the freeform block is probably wrong currently - I will look into it.
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?
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. |
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. |
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