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 various polish #3717

Merged
merged 8 commits into from Nov 29, 2017
Merged

Polish various polish #3717

merged 8 commits into from Nov 29, 2017

Conversation

jasmussen
Copy link
Contributor

This PR does a few things:

  • It makes it so that the Publish button doesn't change label when it's just saving. This likely needs more work, as we actually do want this label to change when you are clicking update on an already published post.
  • It adds bottom padding to the post, which makes for a better experience on longer posts — as you near the end, there's space to scroll down so the Inserter doesn't sit too snuggly near the bottom of the post.
  • It fixes issues with the Placeholder block paddings and margins, so it now looks exactly like the text block it's supposed to emulate. It no longer jumps in paddings.
  • It polishes the toolbar on the Classic Text block, so it's also no longer jumpy.
  • It colorizes the little arrow on the left in the admin menu white, so it matches the body content.

Screenshots

screen shot 2017-11-29 at 13 14 32

screen shot 2017-11-29 at 13 14 44

screen shot 2017-11-29 at 13 14 53

Joen Asmussen added 5 commits November 29, 2017 10:43
This probably needs to be smarter, as it actually does need to change the label when actually _updating_ a post — so not autosave, but when clicking the "Update" button.
This ensures that on long posts, the plus doesn't sit too snugly against the bottom.
This puts the now all flat block-docked toolbar snug against the top editor bar when sticky invokes.

It also fixes jumpiness with the classic text toolbar.
@jasmussen jasmussen self-assigned this Nov 29, 2017
@codecov
Copy link

codecov bot commented Nov 29, 2017

Codecov Report

Merging #3717 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #3717   +/-   ##
=======================================
  Coverage   37.45%   37.45%           
=======================================
  Files         277      277           
  Lines        6707     6707           
  Branches     1222     1222           
=======================================
  Hits         2512     2512           
  Misses       3536     3536           
  Partials      659      659
Impacted Files Coverage Δ
editor/components/post-publish-button/label.js 87.5% <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 0ad2621...1685c50. Read the comment docs.

@mcsf
Copy link
Contributor

mcsf commented Nov 29, 2017

Nice changes! Things look good to me, just wondering if some of the magic numbers in the CSS should become proper variables.

@mtias mtias added the Design label Nov 29, 2017
@jasmussen
Copy link
Contributor Author

Nice changes! Things look good to me, just wondering if some of the magic numbers in the CSS should become proper variables.

I hear you, and I have a separate to-do item to look at converting more stuff to variables separately, because it's a more holistic task given the recent changes in block border widths and paddings. I think I can clean up a lot.

But the number for the double-stacked Classic Text toolbar is hard to convert to variables, as it's definitely an entirely custom toolbar type. I'll look at it, but I prefer to do that separately.

@mtias
Copy link
Member

mtias commented Nov 29, 2017

Going to merge this.

@mtias mtias merged commit 4491055 into master Nov 29, 2017
@mtias mtias deleted the polish/polish branch November 29, 2017 13:36
@gziolo
Copy link
Member

gziolo commented Nov 29, 2017

screen shot 2017-11-29 at 14 42 58

Please note the inserter under Toolbar. We might want to improve experience around that.

@gziolo
Copy link
Member

gziolo commented Nov 29, 2017

Abother one:

screen shot 2017-11-29 at 14 50 56

@@ -1,21 +1,37 @@
$empty-paragraph-height: $text-editor-font-size * 4;

.editor-default-block-appender {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think those styles should be moved out of the editor/components directory. Because they only make sense in the edit-post layout (not in p2 for example). In editor/components we should think of the components (and styles) as reusable components that should look good when inserting them but if we want to position them properly etc... it should be done in the "layout" IMO.

In general, I do this in the. modes/visual-editor/style.scss by specifying the parent .editor-visual-editor +.editor-default-block-appender {

@jasmussen
Copy link
Contributor Author

Please note the inserter under Toolbar. We might want to improve experience around that.

I think that should be opened as a separate issue.

I'll try and fix the stylesheet changes you suggested Riad.

jasmussen pushed a commit that referenced this pull request Dec 1, 2017
This is a tiny PR to address feedback from #3717 (review). It puts CSS styles where they belong, for component reusability.
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