/gutenberg

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Polish quick toolbar #2151

Closed
wants to merge 3 commits into
from

Conversation

Reviewers

@aduth aduth

@iseulde iseulde

@youknowriad youknowriad

@mtias mtias

Assignees

@jasmussen jasmussen

Labels
[Status] In Progress [Type] Question
Projects
None yet
Milestone
No milestone
3 participants
@jasmussen @aduth @iseulde
@jasmussen
Collaborator

jasmussen commented Aug 2, 2017 • edited

This PR bunches together all the quick toolbar groups, and softens the drop shadow:

screen shot 2017-08-02 at 11 58 43

I liked the separate groups before. It made it very clear, for example, that alignments were a group where if you toggled one, the other was untoggled. But at the same time this benefit may largely be theoretical, and they are still grouped — separated by a vertical line. By grouping them together, aside from getting a little more space, we simplify the visual silhuette, which has been brought up a number of times.

Thoughts?

Edit: reverted a table aspect of this PR and made it separate.

@jasmussen jasmussen self-assigned this Aug 2, 2017

@jasmussen jasmussen requested review from youknowriad, mtias, and iseulde Aug 2, 2017

@jasmussen jasmussen referenced this pull request Aug 2, 2017

Open

Editor: Try a fixed block toolbar #2148

@jasmussen jasmussen added a commit that referenced this pull request Aug 2, 2017

@jasmussen jasmussen Add responsive CSS to the table out of the box 
This fixes #1742.

It is also extracted from #2151 because I should know better.

In this PR, if there isn't room for the table it scrolls horizontally. This works in the admin and the theme, and I think it works reasonably well. Still it might be controversial — what might be the challenges?
 2287aaa

@jasmussen jasmussen referenced this pull request Aug 2, 2017

Merged

Add responsive CSS to the table out of the box #2152

@codecov

codecov bot commented Aug 2, 2017 • edited

Codecov Report

Merging #2151 into master will decrease coverage by 0.04%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2151      +/-   ##
==========================================
- Coverage   20.33%   20.28%   -0.05%     
==========================================
  Files         136      136              
  Lines        4274     4283       +9     
  Branches      724      728       +4     
==========================================
  Hits          869      869              
- Misses       2872     2877       +5     
- Partials      533      537       +4
Impacted Files Coverage Δ
editor/modes/visual-editor/block.js 0% <ø> (ø) ⬆️
blocks/library/table/index.js 36.36% <ø> (ø) ⬆️
editor/sidebar/post-status/index.js 0% <0%> (ø) ⬆️

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 fef2904...bf9f8c0. Read the comment docs.

jasmussen added some commits Aug 2, 2017

@jasmussen jasmussen Build some responsive CSS into the table 
This fixes #1742.
 25102d6
@jasmussen jasmussen Reduce the quick toolbar silhuette 
There have been legitimate concerns about the quick toolbar being too visually heavy. This commit chunks all the toolbar groups snugly together, and reduces the visual silhuette with a softer drop shadow.

Let's discuss this.
 b8e352b
@jasmussen jasmussen Revert table CSS. 
Put that in a separate PR.

All checks have failed

2 failing checks
continuous-integration/travis-ci/pr — The Travis CI build failed
Details
continuous-integration/travis-ci/push — The Travis CI build failed
Details
bf9f8c0
@jasmussen
Collaborator

jasmussen commented Aug 2, 2017

Reverted the table CSS, moved to a separate PR, #2152, and rebased.

@jasmussen jasmussen added [Status] In Progress [Type] Question labels Aug 2, 2017

@jasmussen jasmussen changed the title from Polish table responsiveness and quick toolbar to Polish quick toolbar Aug 2, 2017

@jasmussen jasmussen referenced this pull request Aug 2, 2017

Open

Consider moving cog and trash to quick toolbar #2156

editor/modes/visual-editor/block.js
- label={ __( 'Toggle extra controls' ) }
- icon="ellipsis"
- />
+ <div className="editor-visual-editor__group">
@iseulde

iseulde Aug 2, 2017

Member

Do we need an extra div for this? From the screenshot it seems we can just remove some margins? Or am I missing something?

@jasmussen

jasmussen Aug 2, 2017

Collaborator

I moved the drop shadow to a parent container, and that container has to wrap inline-block, and not be full width like the block controls container is.

But I agree it's not super elegant. I'll think about how this can be improved.

components/toolbar/style.scss
@@ -1,7 +1,7 @@
.components-toolbar {
margin: 0;
border: 1px solid $light-gray-500;
- box-shadow: $shadow-popover;
+ //box-shadow: $shadow-toolbar;
@aduth

aduth Aug 3, 2017

Member

Remove commented line?

@mtias mtias added a commit that referenced this pull request Aug 4, 2017

@jasmussen @mtias jasmussen + mtias Add responsive CSS to the table out of the box (#2152) 
This fixes #1742.

It is also extracted from #2151 because I should know better.

In this PR, if there isn't room for the table it scrolls horizontally. This works in the admin and the theme, and I think it works reasonably well. Still it might be controversial — what might be the challenges?
 d95faa5

This was referenced Aug 7, 2017

Open
Open
Open

@jasmussen jasmussen added a commit that referenced this pull request Aug 11, 2017

@jasmussen jasmussen Reduce quick toolbar silhuette 
This compacts the quick toolbar, and softens the shadow. It is a new version of #2151, but is essentially the same code.

The purpose of this is to visually simplify the quick toolbar so it doesn't feel so heavy. It also unifies the look from mobile to desktop, and frees up some space.

The feedback on the previous PR still stands — there's an extra div here. But even after sleeping on it, I can't think of a better way to do this. The thing is, we are moving the drop shadow from each of the three groups into a single group that wraps them all. This has to be an inline-block so it's no wider than the toolbars themselves, but the parent element has to be block so it can have position sticky.

Props to @paaljoachim for actually suggesting this a long time ago. 💖
 595efd8

@jasmussen jasmussen referenced this pull request Aug 11, 2017

Merged

Reduce quick toolbar silhuette #2359

@jasmussen
Collaborator

jasmussen commented Aug 11, 2017

Closing this in favor of #2359. This was too hard to rebase.

@jasmussen jasmussen closed this Aug 11, 2017

@jasmussen jasmussen deleted the polish/tables-quick-toolbar branch Aug 11, 2017

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