-
-
Notifications
You must be signed in to change notification settings - Fork 833
Tabs: Remove margin-bottom being used to hide border. #3521
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
base: main
Are you sure you want to change the base?
Tabs: Remove margin-bottom being used to hide border. #3521
Conversation
🦋 Changeset detectedLatest commit: cf44ae4 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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
f56749a to
63c49b6
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.
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. |
| ⚠️ Potentially breaking change: Tabs don't use margin-bottom to | ||
| hide border anymore. We also remove the bottom border from `.tab`. | ||
|
|
||
| If you want to preserve the previous styling, you can add the following | ||
| custom CSS to your site: | ||
|
|
||
| ``` | ||
| 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 was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great @shubham-padia — just suggesting a few tweaks for the changelog:
| ⚠️ Potentially breaking change: Tabs don't use margin-bottom to | |
| hide border anymore. We also remove the bottom border from `.tab`. | |
| If you want to preserve the previous styling, you can add the following | |
| custom CSS to your site: | |
| ``` | |
| 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; | |
| } | |
| ``` | |
| Fixes an issue where a vertical scrollbar could be displayed on the Starlight `<Tabs>` component when zooming the page | |
| ⚠️ **Potentially breaking change:** The `<Tabs>` component no longer uses `margin-bottom` and `border-bottom` to highlight the current tab. This is now done with a `box-shadow`. If you have custom styling for your tabs, you may need to update it. | |
| If you want to preserve the previous styling, you can add the following custom CSS to your site: | |
| ``` | |
| 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; | |
| } | |
| ``` |
HiDeoo
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, good catch. We probably need the |
delucis
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
| display: flex; | ||
| align-items: center; | ||
| gap: 0.5rem; | ||
| line-height: var(--sl-line-height-headings); | ||
| padding: 0.275rem 1.25rem; | ||
| text-decoration: none; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| --sl-tab-color-border: var(--sl-color-gray-5); | |
| display: flex; | |
| align-items: center; | |
| gap: 0.5rem; | |
| line-height: var(--sl-line-height-headings); | |
| padding: 0.275rem 1.25rem; | |
| text-decoration: none; | |
| box-shadow: 0 2px 0 var(--sl-tab-color-border); |
| overflow-wrap: initial; | ||
| } | ||
| .tab [role='tab'][aria-selected='true'] { | ||
| color: var(--sl-color-white); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| color: var(--sl-color-white); | |
| --sl-tab-color-border: var(--sl-color-text-accent); | |
| color: var(--sl-color-white); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that is pretty much what I had in mind. Just tested locally and this works as expected (just took the liberty of editing this suggestion as it was suggesting to replace the color: var(--sl-color-white); line instead of appending after it.
| .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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| box-shadow: 0 2px 0 var(--sl-color-text-accent); |



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.