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

add a shouldShowCropGuttersIfApplicable query param #4343

Merged
merged 1 commit into from
Oct 8, 2024

Conversation

twrichards
Copy link
Contributor

@twrichards twrichards commented Oct 1, 2024

… and only show crop gutters* if true (plus the existing conditions for showing the gutters).

This will be used by https://github.com/guardian/flexible-content/pull/4974

*the crop gutters were introduced in #4331

Copy link

github-actions bot commented Oct 1, 2024

@twrichards twrichards force-pushed the only-display-crop-gutters-via-url-param branch from e91ec84 to ddf479c Compare October 7, 2024 09:02
@twrichards twrichards changed the title WIP add a shouldShowCropGuttersIfApplicable query param add a shouldShowCropGuttersIfApplicable query param Oct 7, 2024
@twrichards twrichards force-pushed the only-display-crop-gutters-via-url-param branch from ddf479c to ef6216c Compare October 7, 2024 09:55
…rop gutters if `true` (and other conditions for showing the gutters)
@twrichards twrichards force-pushed the only-display-crop-gutters-via-url-param branch from ef6216c to 2ac4d87 Compare October 7, 2024 09:57
@twrichards twrichards marked this pull request as ready for review October 7, 2024 09:59
@twrichards twrichards requested review from a team as code owners October 7, 2024 09:59
Copy link
Contributor

@bryophyta bryophyta left a comment

Choose a reason for hiding this comment

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

haven't run this, but I've read through the code and the changes look good to me 👍

twrichards added a commit that referenced this pull request Oct 7, 2024
…GNM users see the cropping gutters when cropping 5:3 which is now limited to only certain scenarios by query param `shouldShowCropGuttersIfApplicable` (thanks to #4343)
twrichards added a commit that referenced this pull request Oct 7, 2024
…GNM users see the cropping gutters when cropping 5:3 which is now limited to only certain scenarios by query param `shouldShowCropGuttersIfApplicable` (thanks to #4343)
Copy link
Member

@andrew-nowak andrew-nowak left a comment

Choose a reason for hiding this comment

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

looks reasonable to me. I think if this was going to be long lived I'd prefer it be more flexible and configurable like customRatio is, so grid doesn't even have to be aware of what "if applicable" is, but for now this works great!

@prout-bot
Copy link

Seen on collections, image-loader (merged by @twrichards 8 minutes and 39 seconds ago) Please check your changes!

@prout-bot
Copy link

Seen on thrall, kahuna (merged by @twrichards 8 minutes and 44 seconds ago) Please check your changes!

@prout-bot
Copy link

Seen on auth, leases, usage (merged by @twrichards 8 minutes and 48 seconds ago) Please check your changes!

@prout-bot
Copy link

Seen on metadata-editor, cropper, media-api (merged by @twrichards 8 minutes and 53 seconds ago) Please check your changes!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants