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

fix(consign): fixes inverted contrast; image placeholders #13452

Merged
merged 1 commit into from
Feb 2, 2024

Conversation

dzucconi
Copy link
Member

@dzucconi dzucconi commented Feb 2, 2024

Closes DIA-438

OK so this was dark mode related, but not in the way I thought.

These backgrounds were previously defined with: background="black"black is of course a valid HTML color, not a themed value. I replaced it with black100 but, surprisingly, background is not theme-aware. We typically use the prop bg, which is.

Additionally I fixed these images which were causing a content shift on page load. I do note that the assets we use are inadequately sized as well: we need much larger images to pull resizes from.

@dzucconi dzucconi requested a review from dariakoko February 2, 2024 13:24
@dzucconi dzucconi self-assigned this Feb 2, 2024
const collumnHeightMediumcreen =
((width - 20 * 2 - 20 * 2 - 20 * 2) / 3) * aspectRatio
const collumnHeightBigScreen =
((width - 40 * 2 - 40 * 2 - 40 * 2) / 3) * aspectRatio
Copy link
Member Author

Choose a reason for hiding this comment

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

Interesting in that, this was a lot of work to do something that is not so hard: https://www.notion.so/artsy/Handling-Images-On-The-Web-In-Force-5c6832bf4b3e431cb4830061ce057399

<Stack
gap={[0.5, 1]}
// Magic numbers to bottom align images
minHeight={[100, 200, 140]}
Copy link
Member Author

Choose a reason for hiding this comment

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

This should be structured so that we do this in 2 rows, eliminating the need for any manual adjustment. But this is fine for now.

@dzucconi dzucconi enabled auto-merge February 2, 2024 13:26
@dzucconi dzucconi merged commit 2b78e0a into main Feb 2, 2024
11 checks passed
@dzucconi dzucconi deleted the dzucconi/fix/DIA-438--sell-contrast branch February 2, 2024 13:32
@artsy-peril artsy-peril bot mentioned this pull request Feb 2, 2024
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.

1 participant