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: Try reorganizing the editor's header #2878

Merged
merged 3 commits into from
Oct 5, 2017

Conversation

youknowriad
Copy link
Contributor

Description

This changes the UI of the Editor's moving all the content related actions to the left and the document related actions to the right.

Screenshots (jpeg or gifs if applicable):

screen shot 2017-10-04 at 16 56 35

@youknowriad youknowriad added the Needs Design Feedback Needs general design feedback. label Oct 4, 2017
@youknowriad youknowriad self-assigned this Oct 4, 2017
@codecov
Copy link

codecov bot commented Oct 4, 2017

Codecov Report

Merging #2878 into master will decrease coverage by 0.02%.
The diff coverage is 3.03%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2878      +/-   ##
==========================================
- Coverage   33.95%   33.92%   -0.03%     
==========================================
  Files         191      190       -1     
  Lines        5690     5695       +5     
  Branches      997      999       +2     
==========================================
  Hits         1932     1932              
- Misses       3180     3183       +3     
- Partials      578      580       +2
Impacted Files Coverage Δ
editor/header/preview-button/index.js 95.23% <ø> (ø)
editor/header/mode-switcher/index.js 0% <0%> (ø) ⬆️
editor/header/index.js 0% <0%> (ø) ⬆️
editor/header/publish-button/index.js 88.46% <100%> (ø)

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 9e84bec...b66aac6. Read the comment docs.

@mtias
Copy link
Member

mtias commented Oct 4, 2017

I'm really liking this.

<ModeSwitcher />
<SavedState />
<Tools />
<div className="editor-header__left">
Copy link
Member

Choose a reason for hiding this comment

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

Can we use a more semantic class? Maybe editor-header__content-tools and editor-header__settings for the right one?

<div className="editor-header__left">
<Inserter position="bottom right" />
<IconButton
className="editor-tools__undo"
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't the namespace be the file component?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep sorry, copy/paste

.editor-publish-button.is-saving:disabled {
position: relative;

// These styles overrides the disabled state with the button primary styles
Copy link
Member

Choose a reason for hiding this comment

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

@jasmussen is this something we need upstream?

Copy link
Contributor

Choose a reason for hiding this comment

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

Good question. I don't know what the best approach is, here — looking at it now I'm worried that we might have a blue disabled button, even if a user has a different admin color scheme.

Long term one would hope we had a single "button-primary" component that has this all baked in, instead of multiple disparate CSS that's competing.

youknowriad and others added 3 commits October 5, 2017 09:14
- Normalize the margins between items
- Tweak the alignment and size of the publish button
- Make the Settings cog have a background when toggled, and match the size of the publish button
@jasmussen
Copy link
Contributor

A lot of this was inspired by @afercia and @hedgefield and #467, particularly the discussions around grouping the save state indicator with the publish button. I'm curious of your thoughts on this design, Andrea.

@afercia
Copy link
Contributor

afercia commented Oct 5, 2017

@jasmussen grouping and separating content related actions and document/editor related actions makes lot of sense to me. It is a nice improvement.

Also the logical sequence:
Is everything saved? Yes > Let me check how it looks. Preview > OK Publish

seems good to me.

Not sure about a couple details and it doesn't address the sidebar toggle issue yet but to me this is an improvement. 🙂 I've probably missed some of the recent changes, quick questions:

  • what about the visual/text mode switcher?
  • what the ellipsis button is for?

@jasmussen
Copy link
Contributor

what about the visual/text mode switcher?
what the ellipsis button is for?

I think this got buried in another ticket or PR, my apologies, but it is related to the PR to allow users to edit the HTML on a per-block basis (#2797). It's not fully fleshed out yet, but basically the ellipsis would be the mode switch:

screen shot 2017-10-05 at 11 28 41

It could potentially also be pluggable, so global actions like for example spell check, could live there.

@afercia
Copy link
Contributor

afercia commented Oct 5, 2017

Ah yes the ellipsis thing is also on #2460
Reading all the considerations about mobile in #2797 separating the two group of controls makes even more sense to me.

@youknowriad youknowriad merged commit a89801b into master Oct 5, 2017
@youknowriad youknowriad deleted the update/reorganize-header branch October 5, 2017 15:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Design Feedback Needs general design feedback.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants