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.

Sign up for GitHub

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

Jump to bottom

Add padding to Sharedaddy buttons on mobile #5888

Merged
merged 1 commit into from
Feb 16, 2017
Merged

Add padding to Sharedaddy buttons on mobile #5888

merged 1 commit into from
Feb 16, 2017

Conversation

stoyan0v
Copy link
Contributor

@stoyan0v stoyan0v commented Dec 15, 2016 •
edited
Loading

Fixes: #2658

Add additional padding to Sharedaddy buttons on mobile, to prevent Google Webmaster Tools reporting mobile usability issues, because the clickable elements are too close.

@jeherve jeherve added [Feature] Sharing Post sharing, sharing buttons [Status] Needs Design Review Design has been added. Needs a review! [Status] Needs Review This PR is ready for review. [Type] Bug When a feature is broken and / or not performing as intended labels Dec 16, 2016
@jeherve jeherve added this to the 4.6 milestone Dec 16, 2016
@jeherve jeherve modified the milestones: 2/17 - February, 4.7.0 - March 2017 Jan 30, 2017
@samhotchkiss samhotchkiss removed this from the 4.7.0 - March 2017 milestone Feb 3, 2017
georgestephanis
georgestephanis reviewed Feb 13, 2017
View reviewed changes
Copy link
Member

@georgestephanis georgestephanis left a comment

Choose a reason for hiding this comment

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

Would this maybe make more sense to do if we detect an interface that supports touch events? That is, do it in Javascript and add a body class like "jp-sharing-input-touch" that would increase the space between?

I worry as this patch wouldn't do anything to help touchscreen Windows laptops or tablets that have widths larger than 767px.

Also, 767 pixels feels a bit arbitrary? Is the idea just to assume anything less than 768px is a touchscreen? cc: @MichaelArestad for dimensions / best practices on that front.

@MichaelArestad
Copy link
Contributor

@georgestephanis Yeah. They're pretty arbitrary in general. iPads used to only be 768px wide so anything less was for largish mobile devices. In other words, the styles apply to anything smaller than iPads.

@stoyan0v stoyan0v force-pushed the add/padding-to-sharedaddy-buttons-on-mobile branch 3 times, most recently from 7756ab0 to d9bc171 Compare February 14, 2017 11:24
@georgestephanis
Copy link
Member

@stoyan0v Thanks for the update -- does this change appease Google's scanner? My only hesitancy is whether their scanner is just scanning for general ui, or if it picks up on this sort of adaptive change.

georgestephanis
georgestephanis requested changes Feb 15, 2017
View reviewed changes
modules/sharedaddy/sharing.js Outdated
@@ -267,6 +267,8 @@ var updateLinkedInCount = function( data ) {
} );
$more_sharing_buttons.data( 'timer', false );
} );
} else {
$( 'body' ).addClass( 'jp-sharing-input-touch' );
Copy link
Member

Choose a reason for hiding this comment

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

It'd probably be a bit more performant to use $( document.body ) here, instead of passing 'body' through the selector engine.

https://developer.mozilla.org/en-US/docs/Web/API/Document/body
https://www.sitepoint.com/jquery-body-on-document-on/

@stoyan0v
Copy link
Contributor Author

Hi @georgestephanis, it may sounds as stupid question, but what does "Google's scanner" means?
I tried to find more information, but without any success. I will appreciate if you can provide more information. Thanks.

@georgestephanis
Copy link
Member

@stoyan0v I meant the Google Webmaster Tools thing that the initial PR referenced.

georgestephanis
georgestephanis approved these changes Feb 15, 2017
View reviewed changes
Copy link
Member

@georgestephanis georgestephanis left a comment

Choose a reason for hiding this comment

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

With the changes, it looks good to me.

It'd be nice to get a question answered before merging, just to confirm it behaves as expected in Google Webmaster Tools.

@georgestephanis georgestephanis removed the [Status] Needs Review This PR is ready for review. label Feb 15, 2017
@jeherve jeherve added the [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. label Feb 15, 2017
@stoyan0v stoyan0v force-pushed the add/padding-to-sharedaddy-buttons-on-mobile branch from 19fa42e to a83c868 Compare February 16, 2017 11:02
@stoyan0v
Copy link
Contributor Author

Thanks for clarification @georgestephanis. I've replaced the body selector as you requested, but I am not sure that the modifications change the test result. I always receive a message that the site is mobile friendly, even without the patch.

Maybe there is some cache, or the padding is not such problem, when the theme is mobile friendly ( I am testing with Twentyseventeen ).

@jeherve jeherve added [Status] Needs Review This PR is ready for review. and removed [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. labels Feb 16, 2017
@dereksmart
Copy link
Member

Looks reasonable and fixes the problem at hand. Merging as George already approved.

@dereksmart dereksmart merged commit 007b10c into Automattic:master Feb 16, 2017
@dereksmart dereksmart removed the [Status] Needs Review This PR is ready for review. label Feb 16, 2017
jeherve added a commit that referenced this pull request Feb 21, 2017
@jeherve
Changelog: add #5888
2dfa24d
dereksmart pushed a commit that referenced this pull request Feb 28, 2017
@jeherve @dereksmart
Jetpack 4.7: Changelog (#6331)
a9adf86 
* Changelog: update stable tag and move changelog to changelog.txt

Also remove old releases from readme.txt to keep the changelog tab short.

* Changelog: add #5883

Also update the filter's docblock to match new version.

* Changelog: add #5938

* Changelog: add #6298

* Changelog: add #3405

* Changelog: add #5941

* Changelog: add #6239

* Changelog: add #6281

* Changelog: add #6303

* Changelog: add #6018

* Changelog: add #6300

* Changelog: add #6296

* Changelog: add #6130

* Changelog: add #6292

* Readme: remove extra "on".

* Changelog: add #6307

* Changelog: add #3297

* Changelog: add #6275

* Changelog: add #6321

* Changelog: add #6297

* Readme: update the support forum link anchor.

Anchor changed when WordPress.org forums were updated to bbPress 2

* Readme: update list of a12s, it wasn't up to date anymore!

* Changelog: add #6338

* Changelog: add #6337

* Changelog: add #6335

* Changelog: add #6333

* Testing List: first version of the 4.7 testing list.

* Changelog: add #6332

* Changelog: add #6325

* Changelog: add #6326

* Changelog: add #6339

* Changelog: add #6342

* Changelog: add #6343

* Changelog: add #6346

* Changelog: add #6347

* Changelog: add #6279

* Changelog: add #6306

* Changelog: add #6312

* Changelog: add #6316

* Changelog: add #6171

* Changelog: add #6317

* Changelog: add #6246

* Changelog: add #6263

* Changelog: add #4220

* Changelog: add #5888

* Changelog: add #3406

* Changelog: add #3637

* Changelog: add #6320

* Changelog: add #5992

* Changelog: add #6322

* Changelog: add #6324

* Changelog: add #6352

* Changelog: add #6355

* Changelog: add #6360

* Changelog: add #6362

* Changelog: add #6369, #6382

* Changelog: add #6370

* Changelog: add #6375

* Changelog: add #6383

* Changelog: add #6384

* Changelog: add #6386

* Changelog: add #6395

* Changelog: add #6403

* Changelog: add #6406

* Changelog: add #6418

* Changelog: add #6419

* Changelog: add #6434

* Changelog: add #6446

* Changelog: add #6006

* Changelog: add #6096

* Changelog: add #6399

* Changelog: fix typo.

@see #6331 (comment)

* Changelog: add #6440

* Changelog: add #6443

* Changelog: add #6445

* Changelog: add #6463

* Changelog: add #6468

* Changelog: add #6471

* Changelog: add #6474

* Changelog: add #6480

* Changelog: add #6497

* Changelog: add #6499

* Changelog: add #6514

* Changelog: add #6267

* Changelog: add #5940

* Changelog: add #6492

* Changelog: add #5281

* Changelog: add #6327

* Changelog: add #6451

* Changelog: add #6525

* Changelog: add #6530
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@georgestephanis georgestephanis georgestephanis approved these changes

Assignees
No one assigned
Labels
[Feature] Sharing Post sharing, sharing buttons [Status] Needs Design Review Design has been added. Needs a review! Touches WP.com Files [Type] Bug When a feature is broken and / or not performing as intended
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Sharing: Add padding when on mobile
7 participants
@stoyan0v @MichaelArestad @georgestephanis @dereksmart @jeherve @samhotchkiss @matticbot