-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Use the "old" editor as the freeform block #1394
Conversation
280bece
to
e4085b4
Compare
70d8308
to
306be79
Compare
While still a work in progress, it would be good to get some feedback as this is quite different from the current implementation. |
Very nice! I might have a few suggestions to making the kitchen sink button look closer to how James did it in his approach, and I'd want to separately see how I could make this responsive. But there's nothing about this, visually, that offends :) Does it work with custom tinymce buttons? (Not that it has to) BTW I noticed that if you accidentally click the space between the toolbar groups and drag even ever so slightly, you invoke multi select. I think this might be an issue in master also, but figured I'd mention. @EphoxJames Compared to your branch, what are your thoughts on this one? Is it better or worse, what are the pros and cons from a code simplicity point of view? Thanks so much. |
@jasmussen Yeah it's still a WP. All works like the current editor, just need to add the filter for the buttons, and it will work with custom MCE buttons as well. Not much work. Regarding the kitchen sink, yes, I'll have see how we can do it better on this branch. Note though that there can be many buttons, and up to 4 toolbars in total (3 additional). We could just merge the additional ones into one, but it will need to wrap in case there are many buttons.
That's only on this branch, because for now, the toolbar is literally part of the content. I'll fix this. |
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.
This is great! I like this solution much more than trying to rewrite all the tool bar components in react (which was where I was going to end up).
The only problem I saw aside from those pointed out by @jasmussen is that the kitchen-sink can scroll off the top of the page:
content_css: false, | ||
fixed_toolbar_container: '#' + id + '-toolbar', | ||
toolbar1: 'formatselect | alignleft aligncenter alignright | bullist numlist blockquote | bold italic strikethrough link | kitchensink', | ||
toolbar2: 'hr wp_more forecolor pastetext removeformat charmap outdent indent undo redo', |
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.
I could not get the wp_more button do do anything. Is it working as intended?
Another minor difference compared to the other gutenberg blocks is that the controls don't hide while typing and show on mouse move. You can probably fix this and the multi-select issue by putting it into the |
Thanks James, appreciate your input. Let's explore this branch further then! 🎉 |
I'm aware of the issues, it's also still a WIP. Thanks for the initial feedback, I'll try to get everything working then. One thing that's kind of annoying is trying put the non-React toolbar in the slots, which are always hiding and showing, while MCE's toolbar is supposed to render just once. That's why I've added it to the content div for now. @EphoxJames Ideas welcome. :) It would we nice to get this to work somehow, then we could also show advanced buttons in the inspector, maybe. :) Kind of like #1383. Cc @jasmussen |
It's important that we consider code simplicity too. If making the toolbars auto hide for this block adds complexity, maybe we are okay with not adding it for now, or if it can't be done in a nice way_ ever_. It's a tradeoff we're willing to make, and not something that should hold up this PR. |
b887e97
to
f73110e
Compare
Okay, I've fixed the wp_more stuff for now here, but ideally should be fixed in core. Will make a patch there. I've included the plugin and toolbar filters that target the old editor, so that should work now. (Try the TinyMCE Advanced plugin). This will still need further CSS tweaking though, as the toolbars will wrap over a certain length. Maybe scroll like it does in Calypso? Also still needs the kitchen sink figured out... Also keep in mind that there is a potential menu bar (I've not added that filter yet): |
52cea45
to
dbeb663
Compare
Sure, okay, but I think we should design for a bit more buttons than that. Additionally there's also a menu bar which will have to go in here as well. We can style the old toolbars in any way we'd like, but I think we should stick with the current layout at least. |
We're sort of in the weeds, design wise, with this one, and I think it would be good to get it stabilized the way you are already doing right now, then merge it in and iterate on it. But yet another alternative is this: It's essentially what you had before, but instead of "growing upwards" like I suggested, we embrace that the toolbar wraps downwards, but we add a gray chrome area to the top border to account for the extra padding we need. I know this seems overly complex, but in my head the gray can help explain the discrepancy margins that happens when you select the block. |
Tracking core issue here: https://core.trac.wordpress.org/ticket/41230 |
@jasmussen To me, that looks more like a bug than what it looks like now. :) Alternatively, we could not raise the toolbars to the some level as other toolbars, or not raise it if "more" is toggled. |
To clarify a bit on the following: I imagine the behavior of this mockup to be a dropdown only. That is, if you don't have enough space to see the list buttons, you select text, click the ellipsis, click the list button, and as soon as you've done that the dropdown closes again. So that mockup is not showing a permanently open tray of extra buttons. Just a temporary one. I like this solution because it scales to almost any amount of buttons. |
I'm fine with whatever moves this forward. :) Feel free to adjust any way you'd like. |
I'll take a look, but I think we might want to merge this in soon and then iterate further in other branches. One thing worth looking at first, though — if you select a freeform block and deselect by clicking on the white area, toolbars and block outlines disappear as they should. But if you deselect by clicking on the gray area of the sidebar, only the toolbar disappears. The block still stays highlighted with block boundaries. |
Another idea, building on your ideas. What if we made an exception to the Gutenberg toolbar style, to truly embrace the "old editor" idea. Basically this: That way, the toolbar could have the same markup it has in the old editor, and buttons inside could wrap as they do today. What do you think @iseulde ? |
That's good to me. That's the same but without the space in between? What if we just drop the lines as well and have it a full width toolbar (as the old editor)? |
Worth noting that buttons at the top, like the switcher, do different things anyway, even though they look the same. It's not a block switcher or block level alignment. |
Well, the vertical lines could be separators, so it's a single div containing all the elements. Then they would flex wrap, right?
We could do that as well. In fact we might even consider keeping the sort order of the buttons the same as in the old editor. Full width is also on the table ;) but it's also the last step away from the gutenberg toolbar style. I think we should try and see if we can do without the full width for now. |
I started a branch off this branch to test out some styling ideas "try/freeform-old-editor-floating-toolbar". It revises the floating toolbar from the first version but uses a "flex-direction: column-reverse" to reverse the toolbar ordering so more than one toolbar can be stacked. The end result looks the same as the first version except the extra toolbars don't scroll off the top of the page. |
18592dc
to
7e06bc2
Compare
@jasmussen Adjusted based on your feedback. I think this is much better now, and more like the old editor. Still needs some small styling tweaks here and there. |
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.
Love it!
I think we're 90% there. I think we should include at least a separator (border-right) on the paragraph dropdown, so it looks like its own thing. Possibly a few more, but if we add that border, it's cool.
7e06bc2
to
7fc5a0c
Compare
Okay, let's merge an iterate. This seems to be a good base. |
@@ -20,30 +20,10 @@ registerBlockType( 'core/freeform', { | |||
category: 'formatting', | |||
|
|||
attributes: { | |||
content: children(), | |||
content: prop( 'innerHTML' ), |
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.
The html
matcher is a shorthand for prop( 'innerHTML' )
https://github.com/aduth/hpq#api
Per effort to discourage treating as DOM node (#624), have considered whether prop
can even be dropped.
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.
Ah, didn't realise, sorry. :)
}, | ||
} ); | ||
|
||
/* eslint-disable */ |
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 be more specific here with what we're disabling (i.e. list rule names) to avoid introducing errors, and/or a comment to explain why we're disabling.
|
||
tag = tag || 'more'; | ||
classname += ' mce-wp-' + tag; | ||
title = tag === 'more' ? 'Read more...' : 'Next page'; |
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 these be translated? Also ellipsis character …
as more semantically meaningful than three dots.
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.
As commented, this is just slightly modified core code... I'm even thinking we should drop this and let it fail for the current WordPress version as it is fixed in trunk and will be in 4.9.
…n-new-tab Link target option on Image Block
This is still a WIP! 🚧