Skip to content
This repository has been archived by the owner on Nov 18, 2024. It is now read-only.

Color problems with the Instagram grid pattern #468

Closed
carolinan opened this issue Sep 30, 2024 · 9 comments · Fixed by #506
Closed

Color problems with the Instagram grid pattern #468

carolinan opened this issue Sep 30, 2024 · 9 comments · Fixed by #506
Assignees
Labels
[Component] Block Patterns [Priority] High Used to indicate top priority items that need quick attention [Type] Bug An existing feature does not function as intended.

Comments

@carolinan
Copy link
Contributor

Is your feature request related to a problem? Please describe.
In the Instagram grid pattern, there is the brand name followed by an @ and a fake username.

The username is a link, but the @ is not, so in some color palettes, the @ and the username have different colors:
image

In two palettes, both the text and the link color has too low contrast compared to the background color:
Midnight:
image
Twilight::
image

In another palette, the text color has a too low contrast compared to the background color:
Evening:
image

In another palette, the link color has a too low contrast compared to the background color:
Sunrise
image

@carolinan
Copy link
Contributor Author

Let's re-open this and try something similar to #528 where the cover block is replaced with a group block that has a section style.

@carolinan carolinan reopened this Oct 10, 2024
@carolinan
Copy link
Contributor Author

carolinan commented Oct 10, 2024

Also let me add that during the review of the translatable strings it was requested that the @ should be inside the link, which is how on trunk today the link text has one color.

@carolinan carolinan added [Priority] High Used to indicate top priority items that need quick attention and removed [Status] In Progress Tracking issues with work in progress labels Oct 10, 2024
@juanfra
Copy link
Member

juanfra commented Oct 10, 2024

I believe the decision to include a cover block there was made because when using a group block, you can't select the aspect ratio, and it wasn't looking square on mobile.

However, wrapping the cover on a group block could work. It may not be ideal, but it is the way I could see this working as expected and using the section styles. I'll open a PR for this soon.

@juanfra
Copy link
Member

juanfra commented Oct 10, 2024

I've been playing with this one, and if we wrap the cover block in the group block the text is not inherited from the group block section style in some cases, but it's defaulted to black. This is something that's coming from the editor. Please see the screencast:

Screen.Recording.2024-10-10.at.15.18.57.mov

As a workaround, I played a bit with removing the cover block and having a minimum height for the group block that serves as the wrapper. I'm picking the minimum height of 297px which is the computed height of the squares when the pattern is viewed on desktop. This will solve the color problems. But, the first square may look taller in some scenarios.

Screen.Recording.2024-10-10.at.15.20.20-lw.mov

At this point, the alternatives are:

  1. Keep it as is. The first square is black (contrast variable) and not using sections styles, and it uses the cover block.
  2. Use the group block without minimum height, and the first square will look tiny on mobile. We can add padding top/bottom so that it doesn't look so short.
  3. Use the group block with a minimum height and have the risk of having the first square too tall in a few scenarios.

@carolinan @beafialho thoughts?

@beafialho
Copy link
Contributor

As a workaround, I played a bit with removing the cover block and having a minimum height for the group block that serves as the wrapper. I'm picking the minimum height of 297px which is the computed height of the squares when the pattern is viewed on desktop. This will solve the color problems. But, the first square may look taller in some scenarios.

Thanks for taking time to think about this @juanfra. From your second screen recording, it seems that the square scales up and down fairly well in most sizes. If it works overall, I think it would be cooler to have the original color and this setting, instead of having it black.

@juanfra
Copy link
Member

juanfra commented Oct 10, 2024

Thanks @beafialho

I created two PR with two similar approaches:

  1. Try: Update instagram grid pattern to use the group (option 1) #550 - This PR uses a group with a minimum height of 297px. The risk of this one is if the pattern is somehow in a narrow space and the first square is too tall.
  2. Try: Update instagram grid pattern to use the group (option 2) #551 - This PR uses a group with some padding top and bottom. The only risk of this one is that it can look too short on mobile, but it should react well to other places.

Can you please try them out and see which one is the one that you prefer? Thanks 🙌

@beafialho
Copy link
Contributor

Thank you, option 1 looks better IMO!

@juanfra
Copy link
Member

juanfra commented Oct 11, 2024

Thanks, Bea. I will merge option 1 and close option 2.

@juanfra
Copy link
Member

juanfra commented Oct 11, 2024

Closing as fixed in #550

@juanfra juanfra closed this as completed Oct 11, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
[Component] Block Patterns [Priority] High Used to indicate top priority items that need quick attention [Type] Bug An existing feature does not function as intended.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants