`.Box-row` corner radius fix

[MatteuSan]

What are you trying to accomplish?

• Fix the .Box-row styling for the conditional corner radii. (As stated in this issue)

What approach did you choose and why?

• Changed the pseudo-class from &:first-of-type and &:last-of-type to &:first-child and &:last-child. This makes sure that the rendering of the said styles are only applied if the .Box-row is the first or the last child of the Box component.

What should reviewers focus on?

• The fix should have the expected behavior, however I do like to perform a few more additional tests for a few cases. For example, if the order of the .Box-body element and .Box-row elements are switched in the markup, will the styles be behaving the same way from the fix.
• If the behavior fails, I suggest playing with the pseudo-class combinations. Maybe we can find a fix that can patch this issue through that.

Are additional changes needed?

• [x] No, this PR should be ok to ship as is. 🚢

[changeset-bot[bot]]

🦋 Changeset detected

Latest commit: 9c43048bd2ade6512c63ec48fe629984141639e9

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@primer/css	Patch

Not sure what this means? Click here to learn what changesets are.

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

[MatteuSan]

PS: Sorry about the branch name, it was meant to be .Box-row corner radius fix. Really sleepy rn lol

[MatteuSan]

Hmm.. I wonder if we even need these border radii because the Box class already comes with rounded corners? 🤔

If the Box class happens to have overflow: hidden; then it shouldn't be a problem if we remove the radii from the Box-row class. However, this can lead to unexpected border behaviors for the component (based on experience coding up a card component; it was a bit of a pain to deal with). If we don't add overflow: hidden; to the component, the radii of the Box-row component will (probably) cause an overflow problem.

For safety measures I think it sort of makes sense to have the conditional radii on the Box-row corners. We just need to find the suitable pseudo-class/es for the job.

[MatteuSan]

@simurai does the testing take this long? the test still isn't finished for some reason

[simurai]

does the testing take this long? the test still isn't finished for some reason

Sorry, a maintainer has to manually run the checks, which I can do now.

[simurai]

Ok, took another look and there is still a the problem with the rounded corners if the Box-row using a <ul>. E.g.

<div class="Box Box--condensed">
  <div class="Box-header"><h3 class="Box-title">Box title</h3></div>
  <ul>
    <li class="Box-row">Box row one</li>
    <li class="Box-row">Box row two</li>
  </ul>
</div>

You could use rounded-0 to remove the rounded corners. Or switch to <div>, then the :first-of-type/:first-child would not be effective on the first Box-row

<div class="Box Box--condensed">
  <div class="Box-header"><h3 class="Box-title">Box title</h3></div>
  <div class="Box-row">Box row one</div>
  <div class="Box-row">Box row two</div>
</div>

Maybe we could change the docs to fix this problem?

[MatteuSan]

@simurai I'll try to find a fix for this. brb

[MatteuSan]

@simurai YO! It's been a while, I've resorted to just use the overflow method I've mentioned before:

If the Box class happens to have overflow: hidden; then it shouldn't be a problem if we remove the radii from the Box-row class.

So far it's been working well for most use cases. Please let me know if I missed any edge cases we need to consider for this fix.

[simurai]

So far it's been working well for most use cases.

This should work in most cases, but I think it will break in some case for github.com. So I'm a bit nervous to add overflow: hidden; to Box.

• Short term: Use <div class="Box-row rounded-0">Box row one</div> or overflow: hidden; in your own app/site.
• Long term: We'll have to refactor the Box component to fix this issue for rounded corners.

[MatteuSan]

@simurai I may have found a fix that doesn't use overflow: hidden;. I added this code in the .Box selector:

*:not(:first-child),
*:not(:last-child),
*:not(:only-child) {
  .Box-row {
    &:first-child {
      --box-row-top-left-radius: 0;
      --box-row-top-right-radius: 0;
    }
  
    &:last-child {
      --box-row-bottom-right-radius: 0;
      --box-row-bottom-left-radius: 0;
    }
  }
}

This checks if there is a wrapper element or not, and checks if it's a first child, last child, or an only child. And if the conditions check out, it doesn't apply radii to the box row. Border radii still gets applied in the box-row element though through this:

.Box-row {
  // ...

  &:first-child {
    border-top-left-radius: var(--box-row-top-left-radius, #{$border-radius});
    border-top-right-radius: var(--box-row-top-right-radius, #{$border-radius});
  }
  
  &:last-child {
    border-bottom-right-radius: var(--box-row-bottom-right-radius, #{$border-radius});
    border-bottom-left-radius: var(--box-row-bottom-left-radius, #{$border-radius});
  }
  
  // ...
} 

If you feel like this solution doesn't check out in a few more cases, please let me know :>

[MatteuSan]

Hello I was wondering if there's an update with the approval of this pr? thanks :>

[github-actions[bot]]

Hi! This pull request has been marked as stale because it has been open with no activity for 60 days. You can comment on the pull request or remove the stale label to keep it open. If you do nothing, this pull request will be closed in 7 days.

[github-actions[bot]]

Hi! This pull request has been marked as stale because it has been open with no activity for 60 days. You can comment on the pull request or remove the stale label to keep it open. If you do nothing, this pull request will be closed in 7 days.

[MatteuSan]

bump?

[simurai]

Note that the .Box styles moved to Primer ViewComponents called BorderBox.

So we're probably gonna deprecate the .Box component at some point and use BorderBox instead. Not sure if the rounded corner bugs also exist there.