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

Standardize block styles cursor on hover #31188

Merged
merged 2 commits into from
May 10, 2021

Conversation

devfle
Copy link
Contributor

@devfle devfle commented Apr 26, 2021

Description

I have unified the cursor of the block styles when hovering. Previously, a text cursor was displayed no matter what content was in there. This leads to confusion in my opinion. Gutenberg also uses pointer cursor for several interactive elements.

How has this been tested?

Builded the newest version of gutenberg with my changes and tested it in several browsers.

Screenshots

Before:
before

After:
after

Types of changes

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • I've tested my changes with keyboard and screen readers.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR (please manually search all *.native.js files for terms that need renaming or removal).

Copy link
Member

@JustinyAhin JustinyAhin left a comment

Choose a reason for hiding this comment

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

I've tested this PR, and it looks fine for me.

@devfle
Copy link
Contributor Author

devfle commented May 1, 2021

I've tested this PR, and it looks fine for me.

Hi! Thank you for testing! do we have to wait for an other reviewer? Or do I have any tasks to do?

@JustinyAhin
Copy link
Member

I've tested this PR, and it looks fine for me.

Hi! Thank you for testing! do we have to wait for an other reviewer? Or do I have any tasks to do?

Yes, I'd say let's wait for another one.

@JustinyAhin JustinyAhin requested review from carolinan and talldan May 3, 2021 07:10
Copy link
Contributor

@carolinan carolinan left a comment

Choose a reason for hiding this comment

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

Hi
Changing the cursor style here would affect the editors and not only the block styles panel.

I think it would be better to limit the change and increase the specificity of the cursor style that is used in the block styles panel, so that the .editor-styles-wrapper cursor style does not override it.

See https://github.com/WordPress/gutenberg/blob/trunk/packages/block-editor/src/components/block-styles/style.scss#L11)

(Also I am not able to build the files, please refresh it so that it is up to date with changes to trunk)

@devfle
Copy link
Contributor Author

devfle commented May 3, 2021

Hi
Changing the cursor style here would affect the editors and not only the block styles panel.

I think it would be better to limit the change and increase the specificity of the cursor style that is used in the block styles panel, so that the .editor-styles-wrapper cursor style does not override it.

See https://github.com/WordPress/gutenberg/blob/trunk/packages/block-editor/src/components/block-styles/style.scss#L11)

(Also I am not able to build the files, please refresh it so that it is up to date with changes to trunk)

Hi,
thanks for your review. However, weighting the CSS rule more doesn't do anything on its own. As long as the element gets the cursor passed directly, the parent style is not passed to the child element (no matter how many CSS classes are placed in front of the element). In my opinion the block-editor-block-preview__container should not set the cursor to "text" anymore, because the component occurs as you said in several places and therefore the styles from the parent element are overwritten everywhere. So you have to override the cursor everytime, when you are using this component + you want to change the cursor to something other than text.

So if we want to specify the whole thing a bit more, I would have to override this cursor: text; rule (just for the block styles). Are you ok with that? if so, I would do that right now.

I will rebase my branch and add the newest commits of the trunk to it.

@devfle devfle force-pushed the fix/change-block-style-cursor branch from d242298 to f04a6de Compare May 3, 2021 10:42
@carolinan
Copy link
Contributor

carolinan commented May 3, 2021

I am not sure we understood each other. My suggestion is to only change the cursor for this component:
/components/block-styles/style.scss and not at .editor-styles-wrapper.

@carolinan carolinan requested a review from jasmussen May 3, 2021 15:49
@devfle devfle requested a review from ellatrix as a code owner May 3, 2021 17:12
@devfle
Copy link
Contributor Author

devfle commented May 3, 2021

I am not sure we understood each other. My suggestion is to only change the cursor for this component:
/components/block-styles/style.scss and not at .editor-styles-wrapper.

I got that. I just thought it made sense to set the value to inherit in all places and have it be inherited by its parent elements.

Thanks for your suggestion. I have adjusted the whole thing now. However, as I mentioned earlier, I had to define a new rule. We don't get this line: https://github.com/WordPress/gutenberg/blob/trunk/packages/block-editor/src/components/block-styles/style.scss#L11
to override the cursor: text from .editor-styles-wrapper.

@jasmussen
Copy link
Contributor

Thanks for the PR! I wasn't able to spin up the PR, and can't add a ton of value over what Carolina already shared. But I'd ask you also test the Patterns tab:

patterns

It uses this CSS which I fear might regress if we land this PR as is:

Screenshot 2021-05-04 at 11 06 57

@devfle
Copy link
Contributor Author

devfle commented May 4, 2021

Thanks for the PR! I wasn't able to spin up the PR, and can't add a ton of value over what Carolina already shared. But I'd ask you also test the Patterns tab:

It uses this CSS which I fear might regress if we land this PR as is:

Screenshot 2021-05-04 at 11 06 57

Hi! I will test the Patterns tab. :)

@devfle
Copy link
Contributor Author

devfle commented May 4, 2021

@jasmussen I checked it again. Nothing is overwritten by my fix. Neither for the Block Patterns, Reusable Blocks and the normal blocks. I have also implemented Carolina's recommendation (see last commit) so that it only applies to the Block Styles. So it should be safe.

Title:
Bildschirmfoto 2021-05-04 um 11 55 13

Preview:
Bildschirmfoto 2021-05-04 um 11 54 51

@jasmussen
Copy link
Contributor

Awesome! I will let Carolina give the green light when she has a moment. Thanks for the work!

@carolinan
Copy link
Contributor

I have a problem with my local environment where git rejects the checkout of this PR, I will need someone else to do a final test and merge.

Copy link
Contributor

@jasmussen jasmussen left a comment

Choose a reason for hiding this comment

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

Very nice, thanks for your work. This is before:

before

This is after:

after

@jasmussen jasmussen dismissed carolinan’s stale review May 6, 2021 08:18

Feedback has been addressed.

@devfle devfle force-pushed the fix/change-block-style-cursor branch from a80ccc4 to 0c85bb1 Compare May 10, 2021 14:32
@devfle
Copy link
Contributor Author

devfle commented May 10, 2021

I have rebased the branch and now are all Checks green. Can we merge this :) ?

@jasmussen jasmussen merged commit 73af997 into WordPress:trunk May 10, 2021
@jasmussen
Copy link
Contributor

Thank you!

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.

Blocks styles preview containers have cursor: text proprerty
4 participants