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

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

Open
simmonsjenna opened this issue Oct 8, 2024 · 3 comments · May be fixed by #791
Open

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

simmonsjenna opened this issue Oct 8, 2024 · 3 comments · May be fixed by #791
Assignees

Comments

@simmonsjenna
Copy link

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
Copy link
Contributor

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

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

With your proposed change, this would become:

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

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
Copy link
Contributor

danielguillan commented Oct 9, 2024

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
Copy link
Author

simmonsjenna commented Oct 9, 2024

@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

@joshfarrant joshfarrant self-assigned this Oct 21, 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 a pull request may close this issue.

3 participants