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 conditional image display for 'Rise Up' title in exhibition card component #745

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ostafinskim
Copy link
Contributor

No description provided.

Copy link
Contributor

@sean-dunwoody sean-dunwoody left a comment

Choose a reason for hiding this comment

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

Hi @ostafinskim ,

It would be helpful if you could add a comment to the code explaining why this was necessary (a link to any appropriate support ticket or Basecamp thread would be helpful).

This looks like a slightly unusual solution to me, but without knowing the context of why this is being asked, it's hard for me to propose alternatives.

I would note that checking the $title value is an unreliable way to execute this code because the client can change this value in the CMS.

I've tentatively approved this, but it would be helpful if you could provide a bit more context (and add the comment to the code as discussed).

@ostafinskim
Copy link
Contributor Author

Hey Sean,

You're right—this isn't an ideal solution. However, for this specific exhibition card displayed in the "Coming Soon" component, I needed to retrieve an image without predefined dimensions in the thumbnail property. By default, all images in this setup have a fixed 416x416 dimension, and at the time, checking the $title was the only ( not ideal ) way to ensure the correct image appeared in the correct card.

That said, since this card is no longer in the "Coming Soon" component, these changes might no longer be necessary.

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

Successfully merging this pull request may close these issues.

2 participants