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

Move post permalink to beneath title on mobile. #16277

Merged
merged 5 commits into from
Jul 7, 2019

Conversation

tellthemachines
Copy link
Contributor

@tellthemachines tellthemachines commented Jun 25, 2019

Description

Un-positioned post permalink in the editor title block for smallest breakpoint, so it flows below the title and its contents can wrap without breaking layout. On larger breakpoints it is still absolutely positioned.

How has this been tested?

Manually tested at different widths and zoom levels on Chrome, Safari (Mac) and IE (Windows 7).

Screenshots

Screen Shot 2019-06-25 at 3 30 37 pm

Types of changes

Bug fix (non-breaking change which fixes an issue)

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.

@tellthemachines tellthemachines added [a11y] Zooming [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). Needs Design Feedback Needs general design feedback. labels Jun 25, 2019
@tellthemachines tellthemachines marked this pull request as ready for review June 25, 2019 07:29
@tellthemachines tellthemachines added the [Type] Bug An existing feature does not function as intended label Jun 25, 2019
@youknowriad youknowriad requested a review from a team June 25, 2019 09:56
@kjellr
Copy link
Contributor

kjellr commented Jun 25, 2019

Thank you for taking this on, @tellthemachines! This seems like a good fix.

I'm seeing just a couple small visual bugs that should be sorted out:

  1. The Permalink box doesn't appear to have the left border any longer:

Screen Shot 2019-06-25 at 2 51 07 PM

  1. Also, I'm seeing some mismatched alignment on breakpoints between 480 and 600px:

Screen Shot 2019-06-25 at 2 52 05 PM

@tellthemachines
Copy link
Contributor Author

Thanks for the feedback @kjellr! Should be all fixed now.
In what conditions did the left border disappear though? I couldn't reproduce that, just saw it slightly shifted to the left.

@kjellr
Copy link
Contributor

kjellr commented Jun 26, 2019

Thank you, @tellthemachines. These latest updates look excellent.

In what conditions did the left border disappear though? I couldn't reproduce that, just saw it slightly shifted to the left.

This was at small breakpoints, under 480px (I saw it using the Chrome device inspector). However, after 6ac67da, this problem has disappeared. 🎉 The margins look great now.

Just one last tiny observation — is it possible to add some top margin (maybe just 8px, via our $grid-size variable) to the edit button when it wraps to its own line? That will space it out a little more naturally:

Currently:
Screen Shot 2019-06-26 at 4 04 45 PM

Suggested:
Screen Shot 2019-06-26 at 4 04 32 PM

Thanks again, this is very close.

@tellthemachines
Copy link
Contributor Author

@kjellr :

is it possible to add some top margin (maybe just 8px, via our $grid-size variable) to the edit button when it wraps to its own line?

There's no way of targeting the button specifically when it wraps, as that depends on how long its text content is; the best I could do was give a 4px top and bottom margin to each of the elements in that container, but that means spacing is slightly increased overall (only at the smallest breakpoint though):

Screen Shot 2019-06-27 at 10 48 16 am

Is that OK?
(By the way, not sure why your button says "Edit" while mine says "Change Permalinks"?)

@talldan
Copy link
Contributor

talldan commented Jun 27, 2019

(By the way, not sure why your button says "Edit" while mine says "Change Permalinks"?)

Yeah, you have to change the permalink format to one that has a post slug (using that Change Permalinks button), and after that the button becomes an edit button.

I had a test of the branch and it looks really good 👍

the best I could do was give a 4px top and bottom margin to each of the elements in that container, but that means spacing is slightly increased overall (only at the smallest breakpoint though)

I was thinking along similar lines. An option would be to reduce the container's top and bottom padding so that the spacing is still the same. Any other ideas @kjellr?

I also noticed the container feels like it has a lot more space at the top than the bottom when it wraps to multiple lines. It seems as though the copy link icon has a lot of padding around it that causes this.

@kjellr
Copy link
Contributor

kjellr commented Jun 27, 2019

I was thinking along similar lines. An option would be to reduce the container's top and bottom padding so that the spacing is still the same. Any other ideas @kjellr?

I think some combination of these is the best bet. I'd recommend:

  • Changing the * > selector to just target .editor-post-permalink__edit and .editor-post-permalink__link. I think those are the only elements it's really needed on.
  • Removing the top margin from that rule, since it adds a bit too much extra space on top of the permalink itself. Instead, we should keep the bottom margin rule and bump that up to $grid-size.
  • Also, change the padding on the whole .editor-post-permalink container from 5px (I'm not sure why that's a non-standard measurement 🤷‍♂) to $grid-size within the mobile breakpoints.

Here's what that ends up looking like:

Screen Shot 2019-06-27 at 11 40 58 AM

Screen Shot 2019-06-27 at 11 43 14 AM

And here's a quick diff of those changes:

diff --git a/packages/editor/src/components/post-permalink/style.scss b/packages/editor/src/components/post-permalink/style.scss
index 749b2115b..fcf8b39c1 100644
--- a/packages/editor/src/components/post-permalink/style.scss
+++ b/packages/editor/src/components/post-permalink/style.scss
@@ -2,7 +2,7 @@
 	display: inline-flex;
 	align-items: center;
 	background: $white;
-	padding: 5px;
+	padding: $grid-size;
 	font-family: $default-font;
 	font-size: $default-font-size;
 	height: 40px;
@@ -30,6 +30,10 @@
 		margin-right: -$border-width;
 	}
 
+	@include break-small() {
+		padding: 5px;
+	}
+
 	button {
 		// Prevent button shrinking in IE11 when other items have a 100% flex basis.
 		// This should be safe to apply in all browsers because we don't want these
diff --git a/packages/editor/src/components/post-title/style.scss b/packages/editor/src/components/post-title/style.scss
index d8d54b8d6..882e089d2 100644
--- a/packages/editor/src/components/post-title/style.scss
+++ b/packages/editor/src/components/post-title/style.scss
@@ -113,9 +113,9 @@
 	top: -2px;
 	width: calc(100% - #{$block-left-border-width});
 
-	> * {
-		margin-top: $grid-size-small;
-		margin-bottom: $grid-size-small;
+	.editor-post-permalink__edit,
+	.editor-post-permalink__link {
+		margin-bottom: $grid-size;
 	}
 
 	@include break-mobile() {
@@ -125,8 +125,8 @@
 		flex-wrap: nowrap;
 		width: auto;
 
-		> * {
-			margin-top: 0;
+		.editor-post-permalink__edit,
+		.editor-post-permalink__link {
 			margin-bottom: 0;
 		}
 	}

It seems as though the copy link icon has a lot of padding around it that causes this.

Also — the reason that has extra padding is because it's actually a button! 😄 As such, I don't think we should remove any of its padding for now:

button

@talldan
Copy link
Contributor

talldan commented Jun 28, 2019

Also — the reason that has extra padding is because it's actually a button! 😄 As such, I don't think we should remove any of its padding for now

It's a pretty secretive button! Agreed about not removing padding then, I suppose it needs to stay above a minimum button size.

@tellthemachines
Copy link
Contributor Author

@kjellr @talldan I adjusted the margins and padding as suggested, but in the end still had to add the bottom margin to all the children of editor-post-permalink, otherwise, when the button is set to "edit" and all elements are on the same line, they will be misaligned:

Screen Shot 2019-07-01 at 10 13 19 am

@mtias
Copy link
Member

mtias commented Jul 1, 2019

I wonder if we should remove this for now on mobile, and later from desktop too. Once the title is a proper block, we could use the block toolbar to place it, or just keep it in the document sidebar.

@kjellr
Copy link
Contributor

kjellr commented Jul 1, 2019

I wonder if we should remove this for now on mobile, and later from desktop too.

I'd be 100% happy with that change. 🙂 The document sidebar seems like the more natural place for this control — we don't really need it here.

@sarahmonster
Copy link
Member

+100 to moving this control to the document sidebar. The current placement is more of a carry-over from the classic editor, but for users who don't already have that mental model, it makes more sense to include this setting with all the other document settings.

The association with document settings is stronger than the association with the title element, and it's a more natural place for users to look to find this setting. Moving it into the sidebar may help make it more discoverable to users, and reinforces the idea that stuff in the main content area is content that appears in your post, where stuff in the sidebar is settings for that content.

@tellthemachines
Copy link
Contributor Author

I wonder if we should remove this for now on mobile, and later from desktop too.

Moving this control to the sidebar sounds great. I do think we should remove it on mobile and desktop at once though, otherwise we risk someone zooming in on the control only to find it has disappeared. For accessibility purposes, mobile is also desktop at a higher zoom level, so moving things from one place to another should be done across all breakpoints simultaneously.

Question, @mtias @kjellr @sarahmonster : are you recommending doing that straightaway on this PR, or would it be better to create a separate issue for it? Asking mainly because I'm unsure of the amount of work involved in that change.

@sarahmonster
Copy link
Member

I do think we should remove it on mobile and desktop at once though

Agreed! More consistency across platforms, especially in the case you reference, is always good.

I'd recommend opening a new issue and PR for this change, so we can keep things as atomic as possible.

@tellthemachines
Copy link
Contributor Author

I have created #16412 for moving the permalink control into the sidebar. Meanwhile, as that piece of work is dependent on the post title being converted into a block, which is still under discussion in #16281, can we move forward with merging this fix?

Copy link
Member

@karmatosed karmatosed left a comment

Choose a reason for hiding this comment

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

I tested this in a few devices and think it's good to merge now. Thanks for working on this @tellthemachines.

@karmatosed karmatosed removed the Needs Design Feedback Needs general design feedback. label Jul 5, 2019
@tellthemachines tellthemachines merged commit a047c54 into master Jul 7, 2019
@tellthemachines tellthemachines deleted the fix/editor-post-permalink-zoomability branch July 7, 2019 23:21
@youknowriad youknowriad added this to the Gutenberg 6.1 milestone Jul 8, 2019
daniloercoli added a commit that referenced this pull request Jul 8, 2019
…rnmobile/track-unsupported-blocks

* 'master' of https://github.com/WordPress/gutenberg:
  Bump plugin version to 6.1.0-rc.1
  Update HTML anchor explaination text (#16142)
  Move post permalink to beneath title on mobile. (#16277)
  Export cloneBlock method to the mobile (#16441)
  Fix inconsistent references to Settings Sidebar (#16138)
  Adds a cache key to the blocks reducer in order to optimize the getBlock selector (#16407)
  Track the parent block to optimize hierarchy selectors (#16392)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants