Skip to content
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

Chrome: Adding a Toggle to switch between fixed/contextual block toolbar #3311

Merged
merged 5 commits into from Nov 6, 2017

Conversation

youknowriad
Copy link
Contributor

@youknowriad youknowriad commented Nov 2, 2017

This PR adds a Toggle in the ellipsis menu to switch back to the contextual block toolbar. This could serve as a A/B test.

screen shot 2017-11-02 at 12 34 55

Todo:

  • Persist this preference

@youknowriad youknowriad self-assigned this Nov 2, 2017
@codecov
Copy link

codecov bot commented Nov 2, 2017

Codecov Report

Merging #3311 into master will decrease coverage by 0.19%.
The diff coverage is 1.29%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #3311     +/-   ##
=========================================
- Coverage   31.29%   31.09%   -0.2%     
=========================================
  Files         232      236      +4     
  Lines        6510     6555     +45     
  Branches     1160     1164      +4     
=========================================
+ Hits         2037     2038      +1     
- Misses       3752     3792     +40     
- Partials      721      725      +4
Impacted Files Coverage Δ
editor/store-defaults.js 100% <ø> (ø) ⬆️
editor/selectors.js 93.9% <0%> (-0.48%) ⬇️
editor/header/header-toolbar/index.js 0% <0%> (ø) ⬆️
editor/navigable-toolbar/index.js 0% <0%> (ø)
editor/header/mode-switcher/index.js 0% <0%> (ø) ⬆️
...or/modes/visual-editor/block-contextual-toolbar.js 0% <0%> (ø)
editor/actions.js 38.29% <0%> (-0.84%) ⬇️
editor/header/index.js 0% <0%> (ø) ⬆️
editor/header/fixed-toolbar-toggle/index.js 0% <0%> (ø)
editor/header/ellipsis-menu/index.js 0% <0%> (ø)
... and 7 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7dc18a0...8b94d0e. Read the comment docs.

@mtias
Copy link
Member

mtias commented Nov 2, 2017

The button block toolbar appears centered:

image

@mtias
Copy link
Member

mtias commented Nov 2, 2017

The wide sizes have some style issues:

image

image

@mtias
Copy link
Member

mtias commented Nov 2, 2017

It'd also be good to restore the position: sticky behaviour when you scroll.

@jasmussen
Copy link
Contributor

Fast work. Was about to mention the sticky position thing.

The only remaining issue I have is that the ellipsis menu toggle feels a bit wrong. I understand that it uses a checkmark when "enabled", but in that case I'd rather we group them together, show both options, and show a checkmark only with one of them. Sort of in this vein:

screen shot 2017-11-02 at 13 17 19

A perhaps faster alternative could be to use the same behavior as the text mode switch. That is, you click it and it changes both the label and icon. So you could have:

[icon] Fix toolbar to block

click that

[icon] Fix toolbar to top

We'd need a new icon, but for now perhaps you could use https://developer.wordpress.org/resource/dashicons/#editor-kitchensink for both?

@youknowriad
Copy link
Contributor Author

  • Fixed the styling issues.
  • I've used the behavior as the text/visual mode for the toggle. We can revisit this separately, providing a generic way to do a "multi-button" toggle like the mockup.
  • Persisted the choice in local storage.

@mtias mtias mentioned this pull request Nov 6, 2017
@mtias
Copy link
Member

mtias commented Nov 6, 2017

Is this good to go?

@youknowriad
Copy link
Contributor Author

Waiting for the signal :)

@mtias
Copy link
Member

mtias commented Nov 6, 2017

:shipit:

@youknowriad youknowriad merged commit 4804d6d into master Nov 6, 2017
@youknowriad youknowriad deleted the add/fixed-toolbar-switch branch November 6, 2017 12:20
@ahmadawais
Copy link
Contributor

Don't you think having it in the settings would be more intuitive instead of the switcher at the top?

@mtias mtias mentioned this pull request Nov 6, 2017
2 tasks
@mtias
Copy link
Member

mtias commented Nov 6, 2017

@ahmadawais in which settings. The Settings → Writing area?

@ahmadawais
Copy link
Contributor

@mtias Yup. Adding it here some where in the settings would be a better idea.

image

@StaggerLeee
Copy link

Maybe you should seriously think about bringing back "Screen Options" button top-right ?
This is perfect place for things like this. Set it once in a year, or 5 years, and do not want to see this option anymore.

@StaggerLeee
Copy link

I would suggest you leave toolbar switch option in stable version. Why not ?
If it is not much code to maintain for you later in the future.

Very useful option for Users to set it according personal preferences.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants