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

Hover fix #10333

Merged
merged 7 commits into from Nov 14, 2018
Merged

Hover fix #10333

merged 7 commits into from Nov 14, 2018

Conversation

@jobthomas
Copy link
Contributor

@jobthomas jobthomas commented Oct 4, 2018

Description

Fixes #10320

How has this been tested?

Tested on iOS 12 Safari - and Firefox Focus

Screenshots

Types of changes

Disabling :hover for mobile

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
@tofumatt
Copy link
Member

@tofumatt tofumatt commented Oct 4, 2018

Any chance we could get before/after screen recording of this in action? That'd make review easier ❤️

@jobthomas
Copy link
Contributor Author

@jobthomas jobthomas commented Oct 4, 2018

Before: https://cld.wthms.co/VbiBYh - slight colour change when putting fingers on items for scrolling

After: http://cld.wthms.co/LhbM5v - I deliberately put my fingers above the menu items while scrolling and no more colour changes.

@tofumatt tofumatt self-requested a review Oct 8, 2018
Copy link
Member

@tofumatt tofumatt left a comment

Shoot; thought I approved this but I must've got distracted by another tab or a shiny object 😅

Thanks for the PR and thanks for the screenshots; they really helped!

I tweaked your comment to point to the PR and this looks good. I'll merge it once Travis is green 😄

@jobthomas
Copy link
Contributor Author

@jobthomas jobthomas commented Nov 14, 2018

Seems like Travis isn't happy ;) Thanks for the update @tofumatt

@tofumatt
Copy link
Member

@tofumatt tofumatt commented Nov 14, 2018

Travis was buggy with this PR and the failures are complaining about PHP files not in this change 🤷‍♂️

Just gonna merge because this works for me locally...

@tofumatt tofumatt merged commit 721dbab into WordPress:master Nov 14, 2018
1 check failed
1 check failed
continuous-integration/travis-ci/pr The Travis CI build failed
Details
@tofumatt tofumatt added this to the 4.4 milestone Nov 14, 2018
@shaunroselt
Copy link

@shaunroselt shaunroselt commented Nov 19, 2018

So I haven't tested this yet, but it sounds sad when reading this.

I actually use my phone (Samsung Galaxy S9+) with Samsung DeX for all of my development work. I hope hovering will still work on my phone. I don't want this disabled at all. It will be awful if :hover is disabled for mobile.

@jobthomas
Copy link
Contributor Author

@jobthomas jobthomas commented Nov 20, 2018

@shaunroselt could you share a specific example of this? With most phones, it's required to actually click something in order to open up a section, not hover over (you can't hover with a finger).

That said, the only change it that it doesn't show up as a different colour on mobile.

@shaunroselt
Copy link

@shaunroselt shaunroselt commented Nov 20, 2018

@shaunroselt could you share a specific example of this? With most phones, it's required to a

@jobthomas You can however hover with a mouse. I have an external monitor, keyboard and mouse plugged into my phone and then I use it like this. So I actually use the :hover on mobile with my mouse.

I plan on slowly upgrading all of my old websites to WordPress 5 and Gutenberg when it comes out as well as do all my future websites in WordPress 5 and Gutenberg. I definitely want hover to keep working on my phone. I have a mouse connected to my phone 70% of the time. So I don't really use a finger at all, but I do use a mouse. I have a Logitech G502 Mouse at home and a Cooler Master MS120 Mouse at work. I use them both with my phone.

@jobthomas
Copy link
Contributor Author

@jobthomas jobthomas commented Nov 20, 2018

Just as reference, the $break-medium is set to 782px. Based on what I can read, the Galaxy S9 is a higher resolution than that, so that would mean this pull request doesn't affect your device. The external monitor should also not be affected by this at all.

All this does is not change the colours of the hover option if the screen is less wide than 782px.

@shaunroselt
Copy link

@shaunroselt shaunroselt commented Nov 20, 2018

Just as reference, the $break-medium is set to 782px. Based on what I can read, the Galaxy S9 is a higher resolution than that, so that would mean this pull request doesn't affect your device. The external monitor should also not be affected by this at all.

All this does is not change the colours of the hover option if the screen is less wide than 782px.

@jobthomas Okay that sounds better. Thanks. I have a 1920x1080 monitor at home and work for my phone. My actual phone resolution is just a bit bigger than 1440p.

Thanks

@jobthomas
Copy link
Contributor Author

@jobthomas jobthomas commented Nov 20, 2018

I have a 1920x1080 monitor at home and work for my phone. My actual phone resolution is just a bit bigger than 1440p.

Nothing to worry then. It's only for devices smaller than that breakpoint.

grey-rsi pushed a commit to OnTheGoSystems/gutenberg that referenced this pull request Nov 22, 2018
* Making sure hover doesn't appeart on mobile

Fixes WordPress#10320
grey-rsi pushed a commit to OnTheGoSystems/gutenberg that referenced this pull request Jan 4, 2019
* Making sure hover doesn't appeart on mobile

Fixes WordPress#10320
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants