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

Polish Classic block. #9680

Merged
merged 3 commits into from Sep 12, 2018

Conversation

Projects
None yet
2 participants
@jasmussen
Contributor

jasmussen commented Sep 7, 2018

The classic block has received a few regressions recently. This PR aims to fix and polish a few.

Notably, the block More menu is now visible on the selected block, and is now part of the block toolbar. Due to the Classic block not having a block toolbar but its own style of toolbar, if we allow the More menu to sit where the block toolbar is, it will overlap and cover buttons once the sticky positioning kicks in. That's why we moved it on the right on the classic block. The visuals of that have been polished so it now looks more integrated.

There was a weird regression where a pressed button would receive a white icon color, which obv. isn't contrasty enough. This fixes that, though would like @iseulde input -- it looks like a skin file adds this white color, maybe we don't need that skin?

Finally, I tweaked some paddings a bit so the toolbar doesn't jump when selected & initialized.

polish classic block

Polish Classic block.
The classic block has received a few regressions recently. This PR aims to fix and polish a few.

Notably, the block More menu is now visible on the selected block, and is now part of the block toolbar. Due to the Classic block not having a block toolbar but its own style of toolbar, if we allow the More menu to sit where the block toolbar is, it will overlap and cover buttons once the sticky positioning kicks in. That's why we moved it on the right on the classic block. The visuals of that have been polished so it now looks more integrated.

There was a weird regression where a pressed button would receive a white icon color, which obv. isn't contrasty enough. This fixes that, though would like @iseulde input -- it looks like a skin file adds this white color, maybe we don't need that skin?

Finally, I tweaked some paddings a bit so the toolbar doesn't jump when selected & initialized.

@jasmussen jasmussen added this to the 3.9 milestone Sep 7, 2018

@jasmussen jasmussen self-assigned this Sep 7, 2018

@jasmussen jasmussen requested review from iseulde and WordPress/gutenberg-core Sep 7, 2018

@@ -115,6 +115,20 @@
}
.editor-block-list__layout .editor-block-list__block[data-type="core/freeform"] {
// Not sure why this is necessary, there seems to be a skin file that overrides this upstream.

This comment has been minimized.

@jasmussen

jasmussen Sep 7, 2018

Contributor

This is from a skin file, it looks like this without it:

screen shot 2018-09-07 at 10 28 24

It's from /wp-includes/js/tinymce/skins/lightgray/skin.min.css

.mce-btn.mce-active button, .mce-btn.mce-active:hover button, .mce-btn.mce-active i, .mce-btn.mce-active:hover i {
    color: white;
}

Should we be overriding or can we de-queue this one?

@jasmussen

jasmussen Sep 7, 2018

Contributor

This is from a skin file, it looks like this without it:

screen shot 2018-09-07 at 10 28 24

It's from /wp-includes/js/tinymce/skins/lightgray/skin.min.css

.mce-btn.mce-active button, .mce-btn.mce-active:hover button, .mce-btn.mce-active i, .mce-btn.mce-active:hover i {
    color: white;
}

Should we be overriding or can we de-queue this one?

@jasmussen jasmussen referenced this pull request Sep 7, 2018

Merged

Add/drag handle #9569

10 of 10 tasks complete
@kjellr

This comment has been minimized.

Show comment
Hide comment
@kjellr

kjellr Sep 10, 2018

Contributor

Looking good! Just one minor observation:

The left border of the more menu looks a little weird when you expand the second line of controls. I wonder if we can actually just hide that border entirely? None of the other icons in the Classic Editor toolbar have a divider line next to them:

screen shot 2018-09-10 at 9 42 06 am

Contributor

kjellr commented Sep 10, 2018

Looking good! Just one minor observation:

The left border of the more menu looks a little weird when you expand the second line of controls. I wonder if we can actually just hide that border entirely? None of the other icons in the Classic Editor toolbar have a divider line next to them:

screen shot 2018-09-10 at 9 42 06 am

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Sep 10, 2018

Contributor

Yep, I was aware of that border thing. The thing is, all the buttons in the editor they wrap — but the ellipsis does not, because it's an overlay. I make sure there's no actual overlap by adding padding to the classic editor bar, but that also brings us this situation where the border doesn't connect. I could add a bottom border, but that might look even weirder.

But the point is, because all the buttons except the ellipsis wrap, I felt like the border was necessary.

Contributor

jasmussen commented Sep 10, 2018

Yep, I was aware of that border thing. The thing is, all the buttons in the editor they wrap — but the ellipsis does not, because it's an overlay. I make sure there's no actual overlap by adding padding to the classic editor bar, but that also brings us this situation where the border doesn't connect. I could add a bottom border, but that might look even weirder.

But the point is, because all the buttons except the ellipsis wrap, I felt like the border was necessary.

@kjellr

This comment has been minimized.

Show comment
Hide comment
@kjellr

kjellr Sep 10, 2018

Contributor

But the point is, because all the buttons except the ellipsis wrap, I felt like the border was necessary.

That makes sense. When the screen gets a little smaller, that divider does help. Speaking of that though, I'm back with another observation. 😄

When the screen is between 600px and 780px, the more icon isn't in alignment with the other icons, and the border doesn't stretch all the way to the bottom of the row:

Taken around 620px:
screen shot 2018-09-10 at 10 05 29 am

Taken around 720px:
screen shot 2018-09-10 at 10 05 18 am

Contributor

kjellr commented Sep 10, 2018

But the point is, because all the buttons except the ellipsis wrap, I felt like the border was necessary.

That makes sense. When the screen gets a little smaller, that divider does help. Speaking of that though, I'm back with another observation. 😄

When the screen is between 600px and 780px, the more icon isn't in alignment with the other icons, and the border doesn't stretch all the way to the bottom of the row:

Taken around 620px:
screen shot 2018-09-10 at 10 05 29 am

Taken around 720px:
screen shot 2018-09-10 at 10 05 18 am

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Sep 10, 2018

Contributor

Ah that's a good point. There's one breakpoint where the classic editor bar becomes "touch friendly", and obviously another when buttons start to wrap.

But we don't actually know when buttons start to wrap, because there can be a varying amount of buttons. So I can (and will) fix the 620px issue, but I still can't really fix the wrapping issue :(

Contributor

jasmussen commented Sep 10, 2018

Ah that's a good point. There's one breakpoint where the classic editor bar becomes "touch friendly", and obviously another when buttons start to wrap.

But we don't actually know when buttons start to wrap, because there can be a varying amount of buttons. So I can (and will) fix the 620px issue, but I still can't really fix the wrapping issue :(

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Sep 12, 2018

Contributor

Found a way forward:

screen shot 2018-09-12 at 11 43 23

screen shot 2018-09-12 at 11 43 28

screen shot 2018-09-12 at 11 46 51

screen shot 2018-09-12 at 11 46 55

Contributor

jasmussen commented Sep 12, 2018

Found a way forward:

screen shot 2018-09-12 at 11 43 23

screen shot 2018-09-12 at 11 43 28

screen shot 2018-09-12 at 11 46 51

screen shot 2018-09-12 at 11 46 55

@kjellr

kjellr approved these changes Sep 12, 2018

This turned out great. LGTM!

@jasmussen jasmussen merged commit 577e47d into master Sep 12, 2018

2 checks passed

codecov/project No report found to compare against
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jasmussen jasmussen deleted the polish/classic-more-menu branch Sep 12, 2018

@saulirajala saulirajala referenced this pull request Oct 1, 2018

Open

Update/freeform embrace editor css styles #10281

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