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

Polish quick toolbar #2151

Closed
wants to merge 3 commits into from
Closed

Polish quick toolbar #2151

wants to merge 3 commits into from

Conversation

jasmussen
Copy link
Contributor

@jasmussen jasmussen commented Aug 2, 2017

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 added a commit that referenced this pull request Aug 2, 2017
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?
@codecov
Copy link

codecov bot commented Aug 2, 2017

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.

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.
Put that in a separate PR.
@jasmussen
Copy link
Contributor Author

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

@jasmussen jasmussen added [Status] In Progress Tracking issues with work in progress [Type] Question Questions about the design or development of the editor. labels Aug 2, 2017
@jasmussen jasmussen changed the title Polish table responsiveness and quick toolbar Polish quick toolbar Aug 2, 2017
label={ __( 'Toggle extra controls' ) }
icon="ellipsis"
/>
<div className="editor-visual-editor__group">
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove commented line?

mtias pushed a commit that referenced this pull request Aug 4, 2017
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?
jasmussen added a commit that referenced this pull request Aug 11, 2017
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. 💖
@jasmussen
Copy link
Contributor Author

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 August 11, 2017 10:49
Tug pushed a commit that referenced this pull request May 12, 2020
…lipped-on-oneplus

Update Gutenberg ref to point to text-clipping fix attempt
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Status] In Progress Tracking issues with work in progress [Type] Question Questions about the design or development of the editor.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants