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

allow for collections using portrait trails #1590

Merged
merged 8 commits into from
Jun 10, 2024

Conversation

dblatcher
Copy link
Contributor

@dblatcher dblatcher commented Jun 5, 2024

What's changed?

Currently, all trail images are required / expected to have a 5:3 (landscape) aspect ratio - as such this currently hardwired into parts of the InputImage component, such as the uri used to open the GridModal specifying the "imageType=landscape" query param (even though the criteria object specifying the expected dimensions is already a prop).

There are plans to allow 4:5 portraint trail images for some containers, so the component needed to be generalised to allow for images matching other criteria to be selected.

This PR doesn't make any user-facing changes, but refactors the InputImage component so it would work as expected if provided a criteria prop representing 4:5 portrait images.

Implementation notes

  • InputImage needs extra state to track the dimension of its replacement image so it can style the image button in the right shape (using an <img> and relying in the native image dimensions instead of background-image css might have removed the need for this, but this seemed like a less fiddly option) - removed the extra state and replaced with a method to extract the values from props.input.value
  • Crops selected from the Grid Modal are now validated against the criteria - in theory this shouldn't be necessary as the query params will have prevented invalid crops being selected,
  • this PR doesn't address some potential pitfalls in allowing different aspect ratio for certain containers eg:
    • what happens if a card with a portrait trail is dragged to another container that doesn't support them?
    • what happens if a container with cards with portrait trail is moved and/or has its type changed?

Testing

As written, there should be no changes in behaviour compared to main.

To test the mechanism locally, edit the value of defaultCardTrailImageCriteria to portraitCardImageCriteria in fronts-client/src/constants/image.ts. With that change:

  • when you click the trail image to select a replacement trail image from the grid, it should require portrait crops instead of landscape
  • pasting a grid image url into the text input should select a portrait crop (if available)
  • When a portrait crop is selected and the "Use replacement image" option is selected, the button will be styled with the appropriate shape.

Setting defaultCardTrailImageCriteria=undefined should allow you to select either 4:5 or 5:3 images.

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
Screenshot 2024-06-05 at 16 41 38

@dblatcher dblatcher changed the title allow portrait trails allow for collections using portrait trails Jun 5, 2024
@dblatcher dblatcher marked this pull request as ready for review June 6, 2024 12:07
@dblatcher dblatcher requested a review from a team as a code owner June 6, 2024 12:07
Copy link
Contributor

@rebecca-thompson rebecca-thompson left a comment

Choose a reason for hiding this comment

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

Works as expected 👍

@dblatcher dblatcher merged commit d5e30c9 into main Jun 10, 2024
9 checks passed
@dblatcher dblatcher deleted the dblatcher/allow-portrait-trails branch June 10, 2024 15:16
@prout-bot
Copy link

Seen on PROD (merged by @dblatcher 16 minutes and 28 seconds ago) Please check your changes!

jonathonherbert pushed a commit that referenced this pull request Jun 19, 2024
* use hard coded constant so all trails are required to be portrait

* allow grid to select either image type, add state for the dimension of the selected replacement image, use to style the ImageContainer

* bug fix? aspect ratio test should use absolute difference

* use the criteria to set the gird url, not a separate prop

* export the default trail image criteria from constants

* remove unused imports

* shorted var name, comments about use of constant criteria

* dont need state for imageDims - can get from props.input.value
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.

3 participants