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

Make carousel more robust against missing data #20101

Merged

Conversation

sgomes
Copy link
Contributor

@sgomes sgomes commented Jun 17, 2021

The carousel currently fails if some crucial data properties are missing in the images it attempts to load. This will make it more robust, allowing it to fall back to other ones.

Note that the experience still won't be perfect, as the images will be opened with their gallery sizes (which are likely too small for the carousel), but unfortunately the carousel has no way of knowing the original, larger image size if the properties are missing.

This PR also restores the pointer cursor in some situations.

Fixes #11191.

Changes proposed in this Pull Request:

  • Make Carousel more robust against missing data in images
  • Make fade helpers more robust against missing elements
  • Add pointer cursor to galleries that open the carousel

Jetpack product discussion

None. Related to a bug in this repo, however: #11191

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

No.

Testing instructions:

The carousel currently fails if some crucial data properties are missing
in the images it attempts to load.

This will make it more robust, allowing it to fall back to other ones.
In addition, it restores the pointer cursor in some situations.

Fixes #11191.
@sgomes sgomes added [Type] Bug When a feature is broken and / or not performing as intended [Feature] Carousel A fullscreen modal appearing when clicking on an image in a gallery or tiled gallery. [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Jun 17, 2021
@sgomes sgomes requested review from jeherve and sdixon194 June 17, 2021 15:10
@matticbot
Copy link
Contributor

Caution: This PR has changes that must be merged to WordPress.com
Hello sgomes! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer and confirm D62990-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

@github-actions github-actions bot added the [Plugin] Jetpack Issues about the Jetpack plugin. https://wordpress.org/plugins/jetpack/ label Jun 17, 2021
@github-actions
Copy link
Contributor

github-actions bot commented Jun 17, 2021

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 🤖


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: July 6, 2021.
  • Scheduled code freeze: June 28, 2021.

@sgomes sgomes added this to the jetpack/9.9 milestone Jun 17, 2021
Copy link
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

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

This seems to test well for me. 🚢

@jeherve jeherve 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 Jun 18, 2021
@sgomes
Copy link
Contributor Author

sgomes commented Jun 21, 2021

Thank you for the review, @jeherve! 👍

@sgomes sgomes merged commit e4b0592 into master Jun 21, 2021
@sgomes sgomes deleted the fix/make-carousel-more-robust-against-missing-properties branch June 21, 2021 13:11
@github-actions
Copy link
Contributor

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

Thank you!

@github-actions github-actions bot removed the [Status] Ready to Merge Go ahead, you can push that green button! label Jun 21, 2021
@sgomes
Copy link
Contributor Author

sgomes commented Jun 21, 2021

r227547-wpcom

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Carousel A fullscreen modal appearing when clicking on an image in a gallery or tiled gallery. [Plugin] Jetpack Issues about the Jetpack plugin. https://wordpress.org/plugins/jetpack/ Touches WP.com Files [Type] Bug When a feature is broken and / or not performing as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Carousel: error thrown in originalDimensions function
4 participants