Tabs: Remove margin-bottom being used to hide border. #3521
Conversation
🦋 Changeset detectedLatest commit: 665b8aa The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
✅ Deploy Preview for astro-starlight ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
delucis
left a comment
delucis
left a comment
There was a problem hiding this comment.
Very good catch and fix @shubham-padia! Nice one.
I agree this probably should be in a minor release, just in case someone customized border width in their theme and the switch to box-shadow could cause issues. Normally for changes like this we also try to include a snippet in the changeset explaining how to “revert” the new style if wanted for whatever reason, so maybe something like:
starlight-tabs .tab {
margin-bottom: -2px;
}
starlight-tabs .tab > [role='tab'] {
border-bottom: 2px solid var(--sl-color-gray-5);
}
starlight-tabs .tab [role='tab'][aria-selected='true'] {
border-color: var(--sl-color-text-accent);
box-shadow: none;
}There’s a recent example of something like that in the changelog entry for v0.36 in case it’s helpful to see.
shubham-padia
force-pushed
the
fix-tab-overflow-on-zoom-out
branch
2 times, most recently
from
63c49b6 to
7033a9c
Compare
|
Updated the PR @delucis ! Let me know if we want this to be part of a patch release instead of minor. |
HiDeoo
left a comment
HiDeoo
left a comment
There was a problem hiding this comment.
Removing the border on .tab > [role='tab'] would end up with a different behavior when the tab list is scrollable.
| Before | After |
|---|---|
 |
 |
I guess this may be fixable with a box-shadow instead of removing the border, but didn't investigate further yet.
Ah, good catch. We probably need the |
delucis
left a comment
delucis
left a comment
There was a problem hiding this comment.
Probably this set of changes is how I’d approach it:
- Control the colour via a custom property
- Set the
box-shadowon the[role="tab"]element - Update the custom property when the tab is selected.
| .tab [role='tab'][aria-selected='true'] { | ||
| color: var(--sl-color-white); | ||
| border-color: var(--sl-color-text-accent); | ||
| box-shadow: 0 2px 0 var(--sl-color-text-accent); |
There was a problem hiding this comment.
| box-shadow: 0 2px 0 var(--sl-color-text-accent); |
shubham-padia
force-pushed
the
fix-tab-overflow-on-zoom-out
branch
2 times, most recently
from
760836b to
c594d2f
Compare
shubham-padia
force-pushed
the
fix-tab-overflow-on-zoom-out
branch
from
c594d2f to
3a32779
Compare
We were setting margin-bottom as -2px when at the same time we had a bottom border of 2px. Using this indirect margin was causing the tab container to be greater in height than it's wrapper when we zoomed out from the default zoom level. This introduced an unnecessary scroll bar. We use a border-bottom of 2px on `.tab` mainly to mark the active tab with `--sl-color-text-accent`, the additional margin-bottom: -2px hides the grey border from the tablist so it doesn't feel like we have a blue bottom border for the active tab sitting on top of a grey border. Using box shadow instead to paint over the bottom border of the tablist is a cleaner idea than doing these margin-bottom shenanigans. Co-authored-by: Chris Swithinbank <[email protected]>
shubham-padia
force-pushed
the
fix-tab-overflow-on-zoom-out
branch
from
3a32779 to
665b8aa
Compare
|
Sorry for the delay, made the requested changes! |
3 participants
Description
We were setting margin-bottom as -2px when at the same time we had a bottom border of 2px. Using this indirect margin was causing the tab container to be greater in height than it's wrapper when we zoomed out from the default zoom level. This introduced an unnecessary scroll bar.
I'm not sure if this PR warrants a minor version bump or not, let me know if it does.