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

Jetpack performance settings #10468

Merged
merged 14 commits into from Sep 5, 2019

Conversation

@planarvoid
Copy link
Contributor

commented Aug 30, 2019

This PR adds Jetpack performance settings based on the desings. There is now a Site accelerator preference which opens a screen with Faster images and Faster static files and a combined Site accelerator switch which turns both of them on/off.

On the main screen you see now Site accelerator, Lazy load images, Ad-free Video Hosting and More. Clicking on More opens a screen where there are all the mentioned options + Jetpack search.

Ad-free video hosting is only available for Jetpack sites with a Premium or Business plan.

Jetpack search is available to Jetpack sites with a business plan or WP.com sites that have the flag isJetpackSearchSupported set to true.

The easiest way to test this is comparing the options in Calypso with the same site to the options in the WPAndroid app. It happens that the Jetpack refuses to set the settings but it happens on both platforms.

To test - with a Jetpack site with a free plan:

  • Go to Site settings
  • Scroll down
  • Under the Performance category you see:
  • Site accelerator
  • Lazy load images
  • More button

To test - with a Jetpack site with a premium plan:

  • Go to Site settings
  • Scroll down
  • Under the Performance category you see:
  • Site accelerator
  • Lazy load images
  • Ad-free Video hosting option
  • More button
  • Click on the More button
  • You see the same options as above
  • Try setting the new options

To test - with a Jetpack site with a business plan:

  • Go to Site settings
  • Scroll down
  • Under the Performance category you see:
  • Site accelerator
  • Lazy load images
  • Ad-free Video hosting option
  • More button
  • Click on the More button
  • You see the same options as above + the Jetpack search option
  • Try setting the new options

To test - with a WPCom site with a business plan:

  • Go to Site settings
  • Scroll down
  • Under the Performance category you see:
  • Site accelerator
  • Lazy load images
  • More button
  • Click on the More button
  • You see the same options as above + the Jetpack search option
  • Try setting the new options

Desings:
jetpack_peformance_settings_android-1

Implementation:
jetpack_performance_settings

Update release notes:

  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.
@planarvoid planarvoid added the Jetpack label Aug 30, 2019
@planarvoid planarvoid added this to the 13.3 milestone Aug 30, 2019
@planarvoid planarvoid requested a review from khaykov Aug 30, 2019
@planarvoid planarvoid self-assigned this Aug 30, 2019
@peril-wordpress-mobile

This comment has been minimized.

Copy link

commented Aug 30, 2019

Warnings
⚠️ PR has more than 500 lines of code changing. Consider splitting into smaller PRs if possible.

Generated by 🚫 dangerJS

@peril-wordpress-mobile

This comment has been minimized.

Copy link

commented Aug 30, 2019

You can test the changes on this Pull Request by downloading the APK here.

@wordpress-mobile wordpress-mobile deleted a comment from wpmobilebot Sep 2, 2019
@wordpress-mobile wordpress-mobile deleted a comment from wpmobilebot Sep 2, 2019
@wordpress-mobile wordpress-mobile deleted a comment from wpmobilebot Sep 2, 2019
@wordpress-mobile wordpress-mobile deleted a comment from wpmobilebot Sep 2, 2019
planarvoid added 3 commits Sep 2, 2019
@khaykov khaykov self-assigned this Sep 3, 2019
Copy link
Member

left a comment

Thanks for the PR, @planarvoid !

It mostly works as expected, but I have couple of comments:

I see an option for "Add-free video hosting" on Personal plan, where it only starts from Premium.

I don't see the Site accelerator option for wpcom site on a business plan.
Inside More it looks like this:
Image from Gyazo

Behavior of the "global" Site Accelerator switch is not consistent with Calypso - over there it stays enabled if at least one child option is selected, on Android it gets disabled unless both options are selected.

I know you followed a design, but the behavior of More button is a bit weird - sometimes there is nothing extra inside it, it just repeats options from previous screen. Wonder, maybe we should hide it where there is nothing extra to show?

Missing release notes :D

private WPSwitchPreference mLazyLoadImagesNested;

// Jetpack media settings
private WPSwitchPreference mAdFreeHosting;

This comment has been minimized.

Copy link
@khaykov

khaykov Sep 3, 2019

Member

Should we rename this one to mAdFreeVideoHosting to be consistent with naming in JetpackSettingsModel?

} else if (index == R.styleable.SummaryEditTextPreference_switchTrackTint) {
mTrackTint = array.getColorStateList(index);
} else if (index == R.styleable.SummaryEditTextPreference_preferenceTextColor) {
mTextColor = array.getResourceId(index, R.color.white);

This comment has been minimized.

Copy link
@khaykov

khaykov Sep 3, 2019

Member

We actually pulling this color from one of the libraries, so if it'll get removed at some point we might be in trouble. I suggest using android.R.color.white instead (in other places too).

This comment has been minimized.

Copy link
@planarvoid

planarvoid Sep 4, 2019

Author Contributor

ah, I didn't know that, good point 👍

android:title="@string/site_settings_site_accelerator"
app:backgroundColorChecked="@color/primary_40"
app:backgroundColorUnchecked="@color/neutral_40"
app:preferenceTextColor="@color/white"

This comment has been minimized.

Copy link
@khaykov

khaykov Sep 3, 2019

Member

As I mentioned, let's switch this to @android:color/white

@planarvoid

This comment has been minimized.

Copy link
Contributor Author

commented Sep 4, 2019

Thanks for the review, @khaykov !

I see an option for "Add-free video hosting" on Personal plan, where it only starts from Premium.

I've fixed this. I thought that there are only three plans (free, premium and business) and I've checked that it's removed on the free plan. Now I'm hiding it for all the plans except premium and business.

I don't see the Site accelerator option for wpcom site on a business plan.

I'm checking Calypso and it doesn't make sense to me. We're setting Jetpack modules for a non-Jetpack site. Anyway, I'll do the same thing within the app.

Behavior of the "global" Site Accelerator switch is not consistent with Calypso - over there it stays enabled if at least one child option is selected, on Android it gets disabled unless both options are selected.

Good catch, I've fixed that.

I know you followed a design, but the behavior of More button is a bit weird - sometimes there is nothing extra inside it, it just repeats options from previous screen. Wonder, maybe we should hide it where there is nothing extra to show?

It is weird, you're right. I'm now hiding the "More" menu when the "Improved search" is not showing (because it's the only extra item in the preferences.

Missing release notes :D

👍 😄

@khaykov
khaykov approved these changes Sep 5, 2019
Copy link
Member

left a comment

Looks good! Thanks for the fixes :)

@khaykov khaykov merged commit e264626 into develop Sep 5, 2019
5 checks passed
5 checks passed
Peril Found some issues. Don't worry, everything is fixable.
Details
ci/circleci: Installable Build Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: strings-check Your tests passed on CircleCI!
Details
ci/circleci: test Your tests passed on CircleCI!
Details
@khaykov khaykov deleted the feature/jetpack_performance_settings branch Sep 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.