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

Fix issues with sticky quick toolbar positioning #2834

Merged
merged 3 commits into from Sep 29, 2017

Conversation

Projects
None yet
3 participants
@jasmussen
Contributor

jasmussen commented Sep 29, 2017

This PR improves the positioning of the sticky toolbar in normal blocks and the Classic Text blocks, which regressed when the general layout was refactored to have multiple scrollable areas.

Additionally, this PR fixes #1983 by pushing the Classic Text toolbar upwards when the kitchen sink is enabled:

classic text block toolbar

@jasmussen jasmussen self-assigned this Sep 29, 2017

@jasmussen jasmussen requested a review from iseulde Sep 29, 2017

@@ -146,6 +146,10 @@
display: block;
}
.freeform-toolbar.has-advanced-toolbar {
margin-top: -85px; /* Pull upwards the classic text toolbar when enabled */

This comment has been minimized.

@youknowriad

youknowriad Sep 29, 2017

Contributor

Is it always 85 pixels? I guess the advanced toolbar could have several rows?

@youknowriad

youknowriad Sep 29, 2017

Contributor

Is it always 85 pixels? I guess the advanced toolbar could have several rows?

This comment has been minimized.

@youknowriad

youknowriad Sep 29, 2017

Contributor

I tried installing TinyMCE advanced, and I see two issues, when activating the plugin, I see the "menu" toolbar pushing the block's toolbar a bit. And when adding an extra row in the settings, It's not showing up as expected too.

screen shot 2017-09-29 at 09 55 11

@youknowriad

youknowriad Sep 29, 2017

Contributor

I tried installing TinyMCE advanced, and I see two issues, when activating the plugin, I see the "menu" toolbar pushing the block's toolbar a bit. And when adding an extra row in the settings, It's not showing up as expected too.

screen shot 2017-09-29 at 09 55 11

This comment has been minimized.

@jasmussen

jasmussen Sep 29, 2017

Contributor

Solid observation, and good point. I don't know what we can do to fix this — I know it seems hacky, but unless we have some way to calculate the height of the fully expanded toolbar, I can't think of a way to "work perfectly" in every toolbar situation. Especially since responsiveness is going to introduce still more issues.

That said, the above seems to work fairly well considering you have three toolbars. All content is editable, all content is visible, and there is room for the toolbars. Since one key philosophy is that unselected block is preview and selected block/edit mode can show more options, although not optimal this doesn't fall outside of the principles.

Since Gutenberg comes with up to two toolbars by default, I feel like this is a decent compromise, especially since three stacked toolbars is an editor design I don't think is very good in the first place, but there for back compat purposes.

What do you think, @karmatosed?

@jasmussen

jasmussen Sep 29, 2017

Contributor

Solid observation, and good point. I don't know what we can do to fix this — I know it seems hacky, but unless we have some way to calculate the height of the fully expanded toolbar, I can't think of a way to "work perfectly" in every toolbar situation. Especially since responsiveness is going to introduce still more issues.

That said, the above seems to work fairly well considering you have three toolbars. All content is editable, all content is visible, and there is room for the toolbars. Since one key philosophy is that unselected block is preview and selected block/edit mode can show more options, although not optimal this doesn't fall outside of the principles.

Since Gutenberg comes with up to two toolbars by default, I feel like this is a decent compromise, especially since three stacked toolbars is an editor design I don't think is very good in the first place, but there for back compat purposes.

What do you think, @karmatosed?

This comment has been minimized.

@youknowriad

youknowriad Sep 29, 2017

Contributor

yep, good points, I'm ok with this tradeoff

@youknowriad

youknowriad Sep 29, 2017

Contributor

yep, good points, I'm ok with this tradeoff

@karmatosed

This comment has been minimized.

Show comment
Hide comment
@karmatosed

karmatosed Sep 29, 2017

Member

👍 to getting this in. I feel that it's better than what we have right now and that's the important thing. It also makes the toolbar seem less annoying to me, which is a good thing as reported by users.

Member

karmatosed commented Sep 29, 2017

👍 to getting this in. I feel that it's better than what we have right now and that's the important thing. It also makes the toolbar seem less annoying to me, which is a good thing as reported by users.

@jasmussen jasmussen merged commit e7f1223 into master Sep 29, 2017

3 checks passed

codecov/project 33.77% remains the same compared to ff1ef2b
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@jasmussen jasmussen deleted the fix/toolbar-positioning branch Sep 29, 2017

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