Skip to content

Conversation

@dylanatsmith
Copy link
Member

@dylanatsmith dylanatsmith commented Aug 15, 2022

What are you trying to accomplish?

Add gap utility classes matching other spacing utils.

This would solve many issues of responsiveness I’ve run into while implementing layouts.

Something like this solves a lot of headaches with setting margins on elements, removing them from first/last elements, adding margin-top but only on small displays, etc.:

<div class="d-flex flex-wrap gap-3">

What approach did you choose and why?

Looped through spacing values and breakpoints and created applicable classes with the g gap prefix.

What should reviewers focus on?

  • Should this be a thing? For what reason should we not add this?
  • Is g- the right prefix? Maybe we’d want to use gap- and preserve g- for something else?
  • The docs copied the Margin page and removed irrelevant parts. What else should be added to this page?
  • Should this be marked as Stable or a different status?

Can these changes ship as is?

We’d probably want to illustrate the docs with some real-world examples. Since gap is a parent property that affects its children we’d want to consider the best way to show that.

  • Yes, this PR does not depend on additional changes. 🚢

@dylanatsmith dylanatsmith requested a review from a team as a code owner August 15, 2022 21:57
@dylanatsmith dylanatsmith requested a review from jonrohan August 15, 2022 21:57
@changeset-bot
Copy link

changeset-bot bot commented Aug 15, 2022

⚠️ No Changeset found

Latest commit: ffd2916

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@dylanatsmith dylanatsmith marked this pull request as draft August 15, 2022 22:00
Copy link
Contributor

@simurai simurai left a comment

Choose a reason for hiding this comment

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

Should this be a thing? For what reason should we not add this?

We chatted about this in our weekly sync and are mostly ok to add gap as a utility so we don't have to inline it in markup.

I remember talking about it in the past and at the time the conclusion was that we're gonna hold off since we plan to add the Stack component. But we still haven't prioritized the Stack component in PVC, so could still take a few months. And it might even help with migration to Stack.

/cc @vdepizzol that might have been in that discussion.

Is g- the right prefix? Maybe we’d want to use gap- and preserve g- for something else?

I was debating about this too. 🤔 I guess g- would match the m- and p- for margin/padding, but I'm slightly in favor of gap-. Mostly because gap- is pretty short and would help explain what this new utility is for.

The docs copied the Margin page and removed irrelevant parts. What else should be added to this page?

Hmm.. maybe we could add some code examples to visualize the gap between items, like in https://primer.style/css/utilities/margin#uniform-margins?

@dylanatsmith
Copy link
Member Author

I remember talking about it in the past and at the time the conclusion was that we're gonna hold off since we plan to add the Stack component. But we still haven't prioritized the Stack component in PVC, so could still take a few months. And it might even help with migration to Stack.

Thanks for the context. I couldn’t remember where we’d landed on that but would love to land this if it’s going to be a while for Stack (and especially if it will help).

I'm slightly in favor of gap-. Mostly because gap- is pretty short and would help explain what this new utility is for.

The more I think about it, the more I lean toward gap. It’s clearer, helps introduce engineers to the concept if they’re not familiar with it, and it’s short enough that it’s worth spelling out.

Hmm.. maybe we could add some code examples to visualize the gap between items, like in https://primer.style/css/utilities/margin#uniform-margins?

Can do.

@dylanatsmith
Copy link
Member Author

maybe we could add some code examples to visualize the gap between items, like in https://primer.style/css/utilities/margin#uniform-margins?

I’ve added a section with some visualisations. I followed the conventions of the margin and padding visualisations but it’s a bit awkward; the .gap- classes are applied to the parents but appear on the children.

It makes sense if you read the rest of the docs and look at the code examples.

What do you think @simurai?

image

@simurai
Copy link
Contributor

simurai commented Aug 19, 2022

but it’s a bit awkward;

Yeah, having the yellow taking up the whole block is a bit confusing. Maybe we could add an extra <div> and then use d-inline-flex so it doesn't expand all the way to the right. Something like:

image

<div class="mt-3">
  <div class="color-bg-attention d-inline-flex gap-3">
    <div class="color-bg-subtle p-1">.gap-3</div>
    <div class="color-bg-subtle p-1">.gap-3</div>
    <div class="color-bg-subtle p-1">.gap-3</div>
  </div>
</div>

@simurai
Copy link
Contributor

simurai commented Aug 19, 2022

I remember talking about it in the past and at the time the conclusion was that we're gonna hold off since we plan to add the Stack component. But we still haven't prioritized the Stack component in PVC, so could still take a few months. And it might even help with migration to Stack.

Thanks for the context. I couldn’t remember where we’d landed on that but would love to land this if it’s going to be a while for Stack (and especially if it will help).

This came up again in Slack and there are concerns about adding the utilities just before we wanna move to the Stack component and the stack-gap variables.

So an alternative could be to not add the gap utilities in Primer CSS, but instead add them to dotcom. Like the .gap-2 utility already exists here.

But I can bring that up again in our next weekly sync.

@dylanatsmith
Copy link
Member Author

Closing this in favour of adding gap- utilities in https://github.com/github/github/pull/233525

@dylanatsmith dylanatsmith deleted the gap branch August 24, 2022 13:20
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