-
Notifications
You must be signed in to change notification settings - Fork 799
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
Moving jetpack_is_mobile into a package #16129
Conversation
This is an automated check which relies on E2E results is available here (for debugging purposes): https://jetpack-e2e-dashboard.herokuapp.com/pr-16129 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was wondering if you could maybe explain the reasoning behind the decision to move jetpack_is_mobile
into a package.
Is there maybe a discussion or a P2 about it so we can better understand it?
What mostly troubles me here is that jetpack_is_mobile
is also widely used in WP.com so porting this change there could be an issue.
Furthermore, if we were to create a package, maybe it would make sense to have something more generic than Mobile
, like User_Agent
for example, since the functionality ported there is not necessarily mobile-specific.
@fgiannar That's certainly a relevant concern, thank you for asking. @gravityrail should be able to provide more context. It's needed for an (yet) undisclosed A8C project. |
We want to reuse this function in a separate project. If there's a better form for extracting it, let me know. Master thread is pbIAo1-g-p2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a few comments, mostly around naming.
There is not a public discussion, this is for a prototype. Beyond the p2 link above, we do have a ticket and project. Basically, the project wants to estimate the viewport size from the user agent. Jetpack's user agent detection is much richer and more reliable than core's so we wanted to have this as a shared package that we can reuse in our own plugin.
I share that concern. Let's leave a compatibility function in-place with the same name, that calls through to the package. That is an approach that we have used before. |
@gravityrail Could you please have another look? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some quick notes after a first review for me:
It seems you committed a package-lock.json
in here. We don't need it so you can remove it I think 👍
Do you think you could look at the linting errors this brings up? We try to keep the /packages
directory free of any errors. I realize a lot of those carry over from the old files, but I think it'd be nice to fix them instead of having to bypass the pre-commit hook every time we have to make changes to those files in the future.
Absolutely, thank you for checking, @jeherve – I will try to push the needed changes tomorrow. |
…tpack into add/jetpack-mobile-package
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot. I only have one comment left.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me. Merging this now.
|
|
* | ||
* @return bool | ||
*/ | ||
public function is_tablet() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change from static is throwing fatals for sites running the p2-breathe theme. https://wordpress.com/theme/p2-breathe
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Follow-up: #16430
Changes proposed in this Pull Request:
jetpack_is_mobile
into a separate package.Is this a new feature or does it add/remove features to an existing part of Jetpack?
Testing instructions:
jetpack_is_mobile
is still usable and returns the same values as before.Automattic\Jetpack\Mobile::is_mobile
works.Proposed changelog entry for your changes:
jetpack_is_mobile
method into a separate package.