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

Old style for classic block #4948

Merged
merged 8 commits into from Apr 13, 2018

Conversation

Projects
None yet
7 participants
@iseulde
Member

iseulde commented Feb 8, 2018

Description

Fixes #4926. Gives the classic block the old look.

One problem: the toolbar will only start showing on first time focus. :( @spocke, would it be possible at all to add a setting to the modern/inline theme to always show the toolbar? It might also be good to make a separate "theme" for this as we're not using quite a bit of logic in it.

How Has This Been Tested?

Create a classic block.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code has proper inline documentation.
@mtias

This comment has been minimized.

Contributor

mtias commented Feb 9, 2018

This is looking good.

Agreed on fixing the first time focus. It actually doesn't work unless you re-focus when you insert via auto-complete:

image

@mtias

This comment has been minimized.

Contributor

mtias commented Feb 9, 2018

Related: #4928.

@iseulde

This comment has been minimized.

Member

iseulde commented Feb 9, 2018

I'll copy over parts of the "inline" theme from TinyMCE in the next commit so we have more control.

@iseulde

This comment has been minimized.

Member

iseulde commented Feb 13, 2018

I don't really see a way of fixing the focus problem without having to copy over a significant amount of code to the classic block to overwrite the theme. It would be nicer if the inline theme just had a setting to show toolbars all the time instead of just when the editor is focussed.

@iseulde

This comment has been minimized.

Member

iseulde commented Feb 13, 2018

Or for createToolbars etc. to be part of TinyMCE's UI module.

@jasmussen

This comment has been minimized.

Contributor

jasmussen commented Mar 7, 2018

Sorry I missed this. This is looking really good.

Here's the toolbar issue in GIF form:

classic

It would be nice if we could address this, but it doesn't feel like the end of the world to me. Unless we can do some simple hack like run an onload focus for each classic block (hacky), I think it might be worth getting this in and iterating.

@jasmussen

This comment has been minimized.

Contributor

jasmussen commented Mar 7, 2018

Am seeing this error, though:

screen shot 2018-03-07 at 10 47 58

Possibly unrelated?

@iseulde

This comment has been minimized.

Member

iseulde commented Mar 10, 2018

@jasmussen I don't like the idea of manually moving around focus when the page initialises, but it could work... cc @afercia

@jasmussen

This comment has been minimized.

Contributor

jasmussen commented Mar 10, 2018

I can live without it, it'd just be nice if we could find a way to show the toolbar on the classic blocks from the start. But can live without it.

@iseulde

This comment has been minimized.

Member

iseulde commented Mar 10, 2018

Rebased, hopefully the error is gone.

Also added the focus and attempted to reset focus to the previous element, but for some reason it still has the caret in the last classic block... TBH I can live with this so the UI shows up while we wait for a setting in TinyMCE.

@iseulde

This comment has been minimized.

Member

iseulde commented Mar 10, 2018

Cc @spocke or @Afraithe, would it be possible to add a setting to the inline mode of the "modern" TinyMCE theme to always show the toolbar (as opposed to show/hide on focus/blur)?

@iseulde iseulde requested a review from WordPress/gutenberg-core Mar 10, 2018

@spocke

This comment has been minimized.

spocke commented Mar 12, 2018

What we could do is to add a new way of initializing the editor. Currently there is inline and iframe mode what's needed here is some form of inplace editing where a div is converted to contenteditable and the ui is appended before that target element. Not exactly sure how that should be exposed though the api though. But this is something we have seen people asking for before being able to convert div elements to editors but have the ui in place and not hide on blur.

@jasmussen

This comment has been minimized.

Contributor

jasmussen commented Mar 21, 2018

I feel like maybe we should merge this in as is, and then open a separate ticket based on #4948 (comment). Thoughts, @gziolo @karmatosed ?

@karmatosed

This comment has been minimized.

Member

karmatosed commented Mar 21, 2018

I feel like maybe we should merge this in as is, and then open a separate ticket based on #4948 (comment). Thoughts, @gziolo @karmatosed ?

Seems like a very solid plan @jasmussen.

@gziolo

This comment has been minimized.

Member

gziolo commented Mar 22, 2018

Have you tested it with more than one instance of the classic editor?
Have you tested it with the classic editor as a block in the middle of the post?
The fact that we set focus on init might bring unexpected behavior. I can test it quickly myself, too.

@gziolo

This comment has been minimized.

Member

gziolo commented Mar 22, 2018

As I expected, the focus stays in the last Classic block in the document after it gets loaded. I don't think this is an expected behavior. We should look into an alternative solution to make this work.

@gziolo gziolo self-assigned this Mar 23, 2018

@gziolo gziolo added this to the 2.6 milestone Mar 29, 2018

@youknowriad youknowriad modified the milestones: 2.6, 2.7 Apr 5, 2018

@iseulde

This comment has been minimized.

Member

iseulde commented Apr 7, 2018

@gziolo See the PR description and comments. No easy solution. Maybe we should just keep on hiding it for now and only update the styles.

@gziolo

This comment has been minimized.

Member

gziolo commented Apr 7, 2018

Yes, let’s do the tweak with styles first. I think we can also test if showing always the border for the classic block would be sufficient. Maybe we don’t need this toolbar when block is not selected.

@gziolo

This comment has been minimized.

Member

gziolo commented Apr 7, 2018

Another idea, after checking the screencast from @jasmussen. Would it help to add a placeholder for the toolbar to avoid this flickering effect?

@gziolo

This comment has been minimized.

Member

gziolo commented Apr 7, 2018

By the way, can we add an icon to convert to blocks inside the toolbar? That would be awesome 😍

@jasmussen

This comment has been minimized.

Contributor

jasmussen commented Apr 7, 2018

Both of those are good ideas. We have a hover label currently, perhaps that could be the "until you select" toolbar.

@iseulde

This comment has been minimized.

Member

iseulde commented Apr 12, 2018

@jasmussen @gziolo Updated. This should be a lot better. Still not perfect as long as we can't create the toolbar immediately.

@iseulde iseulde requested review from gziolo and jasmussen Apr 12, 2018

@iseulde

This comment has been minimized.

Member

iseulde commented Apr 12, 2018

By the way, can we add an icon to convert to blocks inside the toolbar? That would be awesome 😍

Sure, but separately please :)

@jasmussen

This comment has been minimized.

Contributor

jasmussen commented Apr 13, 2018

Nice work Ella, here's a GIF:

classic

I pushed a little polish so it looks like this:

classic now

That included removing the hover label. This label is likely to go through some iteration regardless, and perhaps become a drag handle, but once we do that it migh sit to the left instead of the top.

With this latest polish, if you approve the code, I think this is good to go! Stellar work, excited to get this in!

@iseulde

This comment has been minimized.

Member

iseulde commented Apr 13, 2018

@jasmussen @gziolo Thanks for the changes and testing!

@gziolo

This comment has been minimized.

Member

gziolo commented Apr 13, 2018

Agreed with @jasmussen, this looks great and make it very clear that we are dealing with Classic block. Thanks for making all the adjustments to this PR, excellent job @iseulde 💯

@gziolo

gziolo approved these changes Apr 13, 2018

@jasmussen is fixing one more small UI glitch and we should merge 👍

@@ -140,7 +140,7 @@ div[data-type="core/freeform"] .editor-block-contextual-toolbar + div {
.freeform-toolbar {
width: auto;
margin: - $block-padding;
margin: -#{ $block-padding + 1 } -#{ $block-padding } -#{ $block-padding } -#{ $block-padding };

This comment has been minimized.

@iseulde

iseulde Apr 13, 2018

Member

I think this code deserves a comment. :)

This comment has been minimized.

@jasmussen

jasmussen Apr 13, 2018

Contributor

😂

On it in a minute.

This comment has been minimized.

@iseulde

iseulde Apr 13, 2018

Member

I also lost the top border now.

screen shot 2018-04-13 at 10 51 43

This comment has been minimized.

@gziolo

gziolo Apr 13, 2018

Member

It works, computes px value :)

@jasmussen

This comment has been minimized.

Contributor

jasmussen commented Apr 13, 2018

So, turns out the past change was due to a rounding error, which I will look at separately. I reverted, so no comment necessary :) Good to go.

@iseulde

This comment has been minimized.

Member

iseulde commented Apr 13, 2018

Okay :) We can iterate in separate PRs :)

@iseulde iseulde merged commit 9a89874 into master Apr 13, 2018

2 checks passed

codecov/project 44.37% (-0.07%) compared to 0a084ad
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@iseulde iseulde deleted the try/classic-old-style branch Apr 13, 2018

@jasmussen

This comment has been minimized.

Contributor

jasmussen commented Apr 13, 2018

🔥 🔥 🔥 🔥

@kjellr kjellr referenced this pull request Aug 1, 2018

Open

Block audit: Classic #8368

1 of 6 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment