primer / brand

React components and Primitives for GitHub marketing websites
https://primer.style/brand
MIT License
74 stars 33 forks source link

[Modification request] Increase spacing between River items on Mobile #783

Closed simmonsjenna closed 1 month ago

simmonsjenna commented 1 month ago

Summary

We received a request (https://github.com/github/marketing-platform-services/issues/3550) to increase the spacing between River sections on mobile to create more separation between these sections and reduce confusion about which copy and images are paired within a River section.

Image

Suggested change

Change the variable used for the padding-top and padding-bottom to: --brand-River-spacing-outer: var(--base-size-40); on mobile.

Image

joshfarrant commented 1 month ago

@simmonsjenna Do you have a preference for the spacing on tablet? Currently, the --brand-River-spacing-outer variable has the following values:

With your proposed change, this would become:

In this case, the spacing drops slightly when we get to tablet viewports. I don't see that as a problem, I just wanted to check in to see if that's what you had in mind, or if you wanted to go 40/40/48, or something like that.

danielguillan commented 1 month ago

We recently merged some changes that reverse the order of the content and visuals on small viewports which should have solved the possible confusion about which copy and images are paired. If we need further tweaks to spacing, we should use the state of the River component as of that PR in order to make the right choices.

Please note that the changes have been merged but haven't been released yet.

simmonsjenna commented 1 month ago

@danielguillan Thanks for the heads-up — if all images will precede text in every River on tablet + mobile, then we don't have to be as heavy with the spacing. I still think something like this would help @joshfarrant:

default (mobile) — var(--base-size-36) 768px — var(--base-size-36) 1080px — var(--base-size-48)

What do you think?

Current Image

With --base-size-36 Image