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

Fix: Cannot access separator block settings by clicking #12950

Closed
wants to merge 1 commit into from

Conversation

emfluenceindia
Copy link

@emfluenceindia emfluenceindia commented Dec 17, 2018

Description

Added a padding to .wp-block-separator class to fix #12080.

How has this been tested?

Tested with in twentyseventeen and twentynineteen themes.

Screenshots

Screencast: https://drive.google.com/file/d/1V8hMsHCJKANl6T3zVOoyNsq6EEor0Kuw/view

Types of changes

New attribute in added .wp-block-separator class

Separator block cannot be selected by clicking on it

N/A

N/A

Checklist:

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

@gziolo gziolo changed the title Proposed fix for #12080 Fix: Cannot access separator block settings by clicking Dec 18, 2018
@gziolo gziolo added [Block] Separator Affects the Separator Block [Type] Bug An existing feature does not function as intended labels Dec 18, 2018
Copy link
Member

@jorgefilipecosta jorgefilipecosta left a comment

Choose a reason for hiding this comment

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

Hi @emfluenceindia thank you a lot for your contribution! From the code point of view, I don't find any problem with this changes. There are some UX changes so I think it would be perfect if @jasmussen and @karmatosed can have a look into this PR.


/* Allows users to understand the clickable area (height) since it is small */
.wp-block-separator:hover {
cursor: pointer;
Copy link
Member

@jorgefilipecosta jorgefilipecosta Dec 18, 2018

Choose a reason for hiding this comment

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

Changing the cursor to a hand may make it look like we can drag & drop things around. While when inside a block that's not the case. Not sure if it is ok or we have a better alternative. cc: @jasmussen in case you have some inputs.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I can understand the thought behind this, but the block should not have separate treatment from any other block.

I think the solution, correctly, is to make the clickable hit area bigger. The fact that we have hover outlines should help.

Copy link
Author

Choose a reason for hiding this comment

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

@jasmussen here is another possible solution. I tested this for both twentyseventeen and twentynineteen. Both show the same result. A few notes in support of adding !important

border-bottom: 2px solid #8f98a1;, for testing I used a different color which never got recognized without adding !important

border-bottom: 2px solid #8f98a1; I tried increasing border thickness to 4px but it never worked without !important

It looks to me that some global hr styling rules are doing the trick. Once I added !important, things started working.

I tried the following modification and it seems to work ok now. Please see the screenshots. @jorgefilipecosta aligning vertically centered seems to be working now as well.

.wp-block-separator {
  border: none;
  border-bottom: 2px solid #8f98a1 !important;
  padding: 15px;
  background: none !important; /* had to add this to get rid of the thick gray patch in twenntyseventeen  due to the padding. */
  position: relative;
  bottom: 10px;
  margin: 1.65em auto; }
  .wp-block-separator:not(.is-style-wide):not(.is-style-dots) {
    max-width: 100px; }

Screenshot of twentyseventeen
2017

Screenshot of twentynineteen
2019

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for the response. It helped me understand why the border is not centered. As you note, it's because it's border-bottom.

I tried a few hacks to center it perfectly and make it more clickable, but found no good solutions. The thing is, a lot of themes built for Gutenberg are now styling the separator assuming it has a border-bottom, so we can't easily change that to, for example, a background color and a height instead.

For that reason, I don't think we should make a patch for just this block.

Instead, we should make a more generic improvement for all blocks. Watch this GIF:

hover

Note how the hover outline appears correctly. But in the space between the content and the blue hover outline, you cannot click to select the block. We should make it so you can.

CC: @aduth, I believe you've talked about this in the past too, can you share any insights into how we might go about this?

Copy link
Author

Choose a reason for hiding this comment

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

Glad that my observation could help! :) And yes, I understand the limitation. As you said it might be a good idea to create a patch for this block in particular as it might make other themes to suffer!

@@ -1,5 +1,6 @@
.wp-block-separator {
border: none;
padding: 10px;
Copy link
Member

Choose a reason for hiding this comment

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

On twenty nineteen theme the line is not vertically centered:

screenshot 2018-12-18 at 09 57 00

Would it be possible to vertically center it?

Copy link
Contributor

Choose a reason for hiding this comment

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

Depending how this is centered vertically — it might be a 2019 specific issue not sure, haven't had time to test yet — we should use $grid-unit instead of 10px.

@gziolo
Copy link
Member

gziolo commented Feb 7, 2019

@jasmussen I noticed you are working on a very similar fix in #13695. Does it duplicate the work done here?

@jasmussen
Copy link
Contributor

@gziolo Yes, correct, it fixes the same issue, and it is inspired by this approach but it addresses the review points from this PR. I forgot about this PR, I thought it might have been closed.

If we land the other PR, let's be sure to give props to emfluenceindia here! Thanks for the ping.

@gziolo
Copy link
Member

gziolo commented Feb 7, 2019

@emfluenceindia do you plan to finish this PR or is it okey to land the other one opened by @jasmussen?

@emfluenceindia
Copy link
Author

@emfluenceindia do you plan to finish this PR or is it okey to land the other one opened by @jasmussen?

I am extremely sorry for my long absence. Actually got overloaded with a lot of things. It is absolutely fine and I won't mind at all if @jasmussen has the fix. :)

@gziolo
Copy link
Member

gziolo commented Feb 7, 2019

I am extremely sorry for my long absence. Actually got overloaded with a lot of things. It is absolutely fine and I won't mind at all if @jasmussen has the fix. :)

Thank you for your contribution in the first place and for the quick response. Let's do it to make sure your work helps to improve the experience with the other PR. I'm looking forward to your future contributions when you have more time :)

@gziolo gziolo closed this Feb 7, 2019
@gziolo gziolo added the [Status] Duplicate Used to indicate that a current issue matches an existing one and can be closed label Feb 7, 2019
@emfluenceindia
Copy link
Author

I am extremely sorry for my long absence. Actually got overloaded with a lot of things. It is absolutely fine and I won't mind at all if @jasmussen has the fix. :)

Thank you for your contribution in the first place and for the quick response. Let's do it to make sure your work helps to improve the experience with the other PR. I'm looking forward to your future contributions when you have more time :)

Thank you for your nice and encouraging words! I would love to contribute more :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Separator Affects the Separator Block [Status] Duplicate Used to indicate that a current issue matches an existing one and can be closed [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot select separator block
4 participants