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

Tiled Gallery Block: allow in Offline Mode. #20534

Merged
merged 6 commits into from
Aug 5, 2021
Merged

Tiled Gallery Block: allow in Offline Mode. #20534

merged 6 commits into from
Aug 5, 2021

Conversation

jeherve
Copy link
Member

@jeherve jeherve commented Aug 3, 2021

Changes proposed in this Pull Request:

Classic Tiled Galleries (created via the Classic editor or within a Classic block in the block editor) can be used even when the site is in Offline mode, ever since #521.

The Tiled Gallery block, however, behaves differently; it requires an active connection. Let's try to be consistent with the feature by:

  • Registering the Tiled Gallery block even when the site is in offline mode.
  • Adding a new utility in the block editor to detect if a site is in offline mode.
  • Short-circuiting and avoid trying to use Photon for Tiled gallery images in the block editor when a site is in offline mode.

Jetpack product discussion

Does this pull request change what data or activity we track or use?

  • No

Testing instructions:

I would suggest testing on two different sites:

  • One that's connected to WordPress.com.
    • The Tiled Gallery block should remain available in the block editor.
    • When publishing a post with a tiled gallery, you should see it use Photon for its images once published.
  • One that's using Offline mode
    • The Tiled Gallery block should now be available in the block editor.
    • Galleries should look good in the editor and on the frontend.
    • Gallery images should not be using Photon.

@jeherve jeherve added [Feature] Comments [Feature] Photon aka "Image CDN". Feature developed in the Image CDN package and shipped in multiple plugins [Block] Tiled Gallery [Pri] Normal [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it labels Aug 3, 2021
@jeherve jeherve self-assigned this Aug 3, 2021
@matticbot
Copy link
Contributor

Caution: This PR has changes that must be merged to WordPress.com
Hello jeherve! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer and confirm D65071-code works as expected before merging this PR. Once this PR is merged, please commit the changes to WP.com. Thank you!
This revision will be updated with each commit to this PR

@jeherve jeherve added this to the jetpack/10.1 milestone Aug 3, 2021
@github-actions github-actions bot added the [Plugin] Jetpack Issues about the Jetpack plugin. https://wordpress.org/plugins/jetpack/ label Aug 3, 2021
@github-actions
Copy link
Contributor

github-actions bot commented Aug 3, 2021 •
edited
Loading

Thank you for your PR!

When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:

  • ✅ Include a description of your PR changes.
  • ✅ All commits were linted before commit.
  • ✅ Add a "[Status]" label (In Progress, Needs Team Review, ...).
  • ✅ Add testing instructions.
  • ✅ Specify whether this PR includes any changes to data or privacy.
  • ✅ Add changelog entries to affected projects

This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖

The e2e test report can be found here. Please note that it can take a few minutes after the e2e tests checks are complete for the report to be available.

Once your PR is ready for review, check one last time that all required checks (other than "Required review") appearing at the bottom of this PR are passing or skipped.
Then, add the "[Status] Needs Team review" label and ask someone from your team review the code.
Once you’ve done so, switch to the "[Status] Needs Review" label; someone from Jetpack Crew will then review this PR and merge it to be included in the next Jetpack release.

Jetpack plugin:

  • Next scheduled release: September 7, 2021.
  • Scheduled code freeze: August 30, 2021.

sdixon194
sdixon194 approved these changes Aug 4, 2021
View reviewed changes
Copy link
Contributor

@sdixon194 sdixon194 left a comment

Choose a reason for hiding this comment

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

This works correctly for me, although I couldn't reproduce the offline issue on latest stable. I also noticed that the photon toggle is still toggle-able, which I feel like I'd expect not to be the case in offline mode?

Set offline by adding define( 'JETPACK_DEV_DEBUG', true ); to a JN's wp-config.

@sdixon194 sdixon194 added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Aug 4, 2021
@jeherve
Copy link
Member Author

jeherve commented Aug 4, 2021

I also noticed that the photon toggle is still toggle-able, which I feel like I'd expect not to be the case in offline mode?
Set offline by adding define( 'JETPACK_DEV_DEBUG', true ); to a JN's wp-config.

Could it be that Photon was active on your site before you switched on Offline mode?
Do you experience similar issues when using Offline mode on a fresh new install, before to even connect to WordPress.com, or on https://localhost?

@sdixon194
Copy link
Contributor

Could it be that Photon was active on your site before you switched on Offline mode?

Nope, I didn't activate photon at all. Same for localhost and on a clean JN install, and before connecting to wpcom. Photon toggle is always available to be activated, and tiled gallery shows up.

@jeherve jeherve mentioned this pull request Aug 5, 2021
Merged
jeherve added a commit that referenced this pull request Aug 5, 2021
@jeherve
Admin: check for offline mode when enabling Photon via main toggle
cf8da10 
The Site Accelerator toggle is a custom toggle that works a bit differently from the standard ModuleToggle component. Since it handles 2 modules at once (Asset CDN and Photon), it needs to check if the 2 modules can actually be toggled on before to activate them. Until now, we checked if the Photon module had an override set via a filter, but we did not check if it was rendered unavailable via Offline mode. This changes that.

This was brought up in #20534 (comment)
@jeherve
Copy link
Member Author

jeherve commented Aug 5, 2021

Ah yes, I was able to reproduce. That's an issue with the toggle. I took care of this bug in #20555.

@jeherve jeherve changed the title Tiled Gallery Block: ensure it can be used when in Offline Mode. Tiled Gallery Block: allow in Offline Mode. Aug 5, 2021
@jeherve jeherve enabled auto-merge (squash) August 5, 2021 08:35
@jeherve jeherve merged commit cdaff07 into master Aug 5, 2021
@jeherve jeherve deleted the update/tiled-gallery-block-offline-mode branch August 5, 2021 08:46
@github-actions github-actions bot removed the [Status] Ready to Merge Go ahead, you can push that green button! label Aug 5, 2021
@github-actions
Copy link
Contributor

github-actions bot commented Aug 5, 2021

Great news! One last step: head over to your WordPress.com diff, D65071-code, and commit it.
Once you've done so, come back to this PR and add a comment with your changeset ID.

Thank you!

@jeherve
Copy link
Member Author

jeherve commented Aug 5, 2021

r229845-wpcom

davidlonjon added a commit that referenced this pull request Aug 5, 2021
@davidlonjon
Merge branch 'master' into fix/boost-linting-php
62a0a99 
* master:
  Update selector for button in Mailchimp block (#20570)
  Jetpack API package (#20531)
  E2E tests: use Allure steps (#20273)
  Tiled Gallery Block: allow in Offline Mode. (#20534)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sdixon194 sdixon194 sdixon194 approved these changes

Assignees

@jeherve jeherve

Labels
[Block] Tiled Gallery [Feature] Comments [Feature] Photon aka "Image CDN". Feature developed in the Image CDN package and shipped in multiple plugins [Plugin] Jetpack Issues about the Jetpack plugin. https://wordpress.org/plugins/jetpack/ [Pri] Normal Touches WP.com Files [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it
Projects
None yet
Milestone
jetpack/10.1
Development

Successfully merging this pull request may close these issues.
3 participants
@jeherve @matticbot @sdixon194