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

Eliminate "AMP Settings" panel from Gallery block and move them to AMP advanced settings #7171

Merged
merged 3 commits into from
Jan 26, 2023

Conversation

thelovekesh
Copy link
Collaborator

@thelovekesh thelovekesh commented Jun 29, 2022

Summary

This PR aims to move AMP Settings panel from the gallery block to advanced settings.

Fixes #4989

Checklist

  • My code is tested and passes existing tests.
  • My code follows the Engineering Guidelines (updates are often made to the guidelines, check it out periodically).

@github-actions
Copy link
Contributor

github-actions bot commented Jun 29, 2022

Plugin builds for 7f3c065 are ready 🛎️!

@thelovekesh thelovekesh marked this pull request as draft June 29, 2022 12:42
@thelovekesh thelovekesh marked this pull request as ready for review June 29, 2022 14:33
@thelovekesh thelovekesh marked this pull request as draft June 29, 2022 14:37
@thelovekesh thelovekesh marked this pull request as ready for review June 29, 2022 14:37
@codecov
Copy link

codecov bot commented Jun 29, 2022

Codecov Report

Merging #7171 (67983f4) into develop (0bf2bbb) will decrease coverage by 0.02%.
The diff coverage is 50.00%.

❗ Current head 67983f4 differs from pull request most recent head 22c4181. Consider uploading reports for the commit 22c4181 to get more accurate results

Impacted file tree graph

@@              Coverage Diff              @@
##             develop    #7171      +/-   ##
=============================================
- Coverage      77.45%   77.43%   -0.03%     
+ Complexity      7123     7116       -7     
=============================================
  Files            275      274       -1     
  Lines          22086    22053      -33     
=============================================
- Hits           17106    17076      -30     
+ Misses          4980     4977       -3     
Flag Coverage Δ
javascript 64.86% <ø> (-0.02%) ⬇️
php 78.12% <50.00%> (-0.03%) ⬇️
unit 78.12% <50.00%> (-0.03%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
assets/src/block-editor/helpers/index.js 23.61% <ø> (-4.79%) ⬇️
src/OptionsRESTController.php 59.78% <ø> (ø)
includes/embeds/class-amp-core-block-handler.php 93.20% <16.66%> (-2.30%) ⬇️
includes/options/class-amp-options-manager.php 90.95% <100.00%> (+0.18%) ⬆️
includes/amp-helper-functions.php 84.89% <0.00%> (-0.03%) ⬇️
...udes/sanitizers/class-amp-pwa-script-sanitizer.php 100.00% <0.00%> (ø)
...zers/class-amp-native-img-attributes-sanitizer.php
includes/sanitizers/class-amp-img-sanitizer.php 97.33% <0.00%> (+2.09%) ⬆️

@thelovekesh thelovekesh self-assigned this Jun 29, 2022
@thelovekesh thelovekesh marked this pull request as draft June 29, 2022 15:11
@thelovekesh thelovekesh marked this pull request as ready for review June 29, 2022 15:12
@thelovekesh
Copy link
Collaborator Author

Failing tests have been fixed in - #7158

@thelovekesh thelovekesh force-pushed the remove/amp-settings-from-gallery-block branch from 67983f4 to 22c4181 Compare July 10, 2022 15:32
@westonruter
Copy link
Member

As I understand, this PR moves the gallery/lightbox settings from an individual block to be a global settings. I don't think will work out because a user may want a carousel for one gallery block but not have it for another block. I realize #4989 is not particularly clear as it was more of a issue for more discovery, but I recall the idea for global settings was whether to control whether the block-specific settings would be exposed. For example, let's say you are using Jetpack and all galleries already have lightboxes. In that case, the checkbox for AMP lightbox is redundant and shouldn't be presented to all users for every single block.

Instead of removing the AMP-specific toggles, can the AMP toggles be moved to the Advanced panel so as to eliminate the AMP Settings panel?

@thelovekesh
Copy link
Collaborator Author

For example, let's say you are using Jetpack and all galleries already have lightboxes. In that case, the checkbox for AMP lightbox is redundant and shouldn't be presented to all users for every single block.

Agree 💯

I will move them to the Advanced Settings 👍🏼

@thelovekesh thelovekesh force-pushed the remove/amp-settings-from-gallery-block branch from f13567f to 364ceee Compare August 2, 2022 14:45
@westonruter westonruter added this to the v2.3.1 milestone Nov 29, 2022
@thelovekesh
Copy link
Collaborator Author

@westonruter Can you give it a round of review?

Copy link
Member

@westonruter westonruter left a comment

Choose a reason for hiding this comment

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

LGTM!

@westonruter westonruter merged commit bbc5c1c into develop Jan 26, 2023
@westonruter westonruter deleted the remove/amp-settings-from-gallery-block branch January 26, 2023 19:48
@westonruter westonruter modified the milestones: v2.3.1, v2.4 Jan 26, 2023
@westonruter westonruter added the Changelogged Whether the issue/PR has been added to release notes. label Feb 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelogged Whether the issue/PR has been added to release notes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Eliminate "AMP Settings" panel from Gallery block
2 participants