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 recrop button for trail images #1660

Merged
merged 1 commit into from
Sep 23, 2024
Merged

add recrop button for trail images #1660

merged 1 commit into from
Sep 23, 2024

Conversation

twrichards
Copy link
Contributor

@twrichards twrichards commented Sep 19, 2024

Related guardian/grid#4331

There appeared to be glaring omission in the functionality around 'overriding trail images' since the existing button only let you 'replace image' (taking you to grid homepage/search in the modal), unlike say composer's 'recrop button' on image elements which at least takes you to the picture that's already selected, meaning that to tweak the crop used for a trail image, the same image would need to be re-found amongst the millions in the grid.

What's changed?

In this PR we add a new 'Recrop image' button alongside the 'Replace image' button to provide the missing functionality...

Before After
image image

... this takes the user to that specific image in crop mode (ready to create a new crop). Thanks to @waisingyiu's #1601 if the container uses 5:4 trails then the crop options will be limited to 5:4 (otherwise restricted to 5:3 when overriding trail images).

It now also passes a seedCropId, in anticipation of grid using it (guardian/grid#4332) for the starting position/dimensions of the new crop (particularly useful when re-cropping a 5:3 into a 5:4).

Although, prompted by the 5:4 trail images situation, this feature is useful in its own right.

Checklist

General

  • 🤖 Relevant tests added
  • ✅ CI checks / tests run locally
  • 🔍 Checked on CODE

Client

  • 🚫 No obvious console errors on the client (i.e. React dev mode errors)
  • 🎛️ No regressions with existing user interactions (i.e. all existing buttons, inputs etc. work)
  • 📷 Screenshots / GIFs of relevant UI changes included

@twrichards twrichards changed the title WIP add recrop button for trail images add recrop button for trail images Sep 20, 2024
@twrichards twrichards marked this pull request as ready for review September 20, 2024 15:10
@twrichards twrichards requested a review from a team as a code owner September 20, 2024 15:10
@paperboyo
Copy link
Contributor

paperboyo commented Sep 20, 2024

Yay, thank you!

if the container uses 5:4 trails then the crop options will be limited to 5:4 (otherwise restricted to 5:3 when overriding trail images)

And if container uses 4:5, it restricts to 4:5, right? (sorry for being pedantic!)

@twrichards
Copy link
Contributor Author

Yay, thank you!

if the container uses 5:4 trails then the crop options will be limited to 5:4 (otherwise restricted to 5:3 when overriding trail images)

And if container uses 4:5, it restricts to 4:5, right? (sorry for being pedantic!)

It should yes. My comment was more an example that it preserves whatever logic was there already.

…existing image ready to create a new crop (with any ratio constraints)
Copy link
Contributor

@dblatcher dblatcher left a comment

Choose a reason for hiding this comment

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

LGMT!
Not one for this PR, but it might be worth adding a recrop button for replacement images as well as on the original trail image.

@twrichards
Copy link
Contributor Author

twrichards commented Sep 23, 2024

Thanks for the review @dblatcher

Not one for this PR, but it might be worth adding a recrop button for replacement images as well as on the original trail image.

Yeah good thought, I did consider that myself at the time but didn't want to change the UX too much - so as you say for a future PR perhaps (also let's see if any users request that once they've had this PR's functionality for a little while)

@twrichards twrichards merged commit f84cfaf into main Sep 23, 2024
12 checks passed
@twrichards twrichards deleted the recrop-trail branch September 23, 2024 10:35
@prout-bot
Copy link

Seen on PROD (merged by @twrichards 6 minutes and 26 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