Update Tabs component design and add tests #159
Conversation
KenanYusuf
commented
Nov 3, 2023
•
KenanYusuf
commented
- Updated the tab style according to the new designs
- Added tests for tabs
- Added Story for tabs
🦋 Changeset detectedLatest commit: a470926 The changes in this PR will be included in the next version bump. This PR includes changesets to release 6 packages
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 |
packages/webui/src/styles/base.css
Outdated
| @@ -5,7 +5,7 @@ | |||
| @tailwind utilities; | |||
| @tailwind variants; | |||
|
|
|||
| html { | |||
| body { | |||
There was a problem hiding this comment.
Switched to body as applying this to the html was causing the REM-based spacing scale to differ from what we have in designs.
There was a problem hiding this comment.
Fair enough. Not sure I can remember why I did this on html in the first place, but just out of curiosity can you share an example of what wasn't working correctly with this, and why the change to body fixes it... I feel like there's a lesson for me to learn here.
There was a problem hiding this comment.
For sure, sorry my comment was a bit vague.
So Tailwind's spacing values use rem units. Rem values are relative to the font-size of the "root" (<html />) element, so if the font-size on the html element is 16px, 0.5rem will compute to 8px. If the html element's font-size is changed to 14px, 0.5rem will compute to 7px.
There are probably some articles that explain it better than I can, but here is a visual example of the impact it has on the computed values:
Before - 14px font-size on the html element
After - 14px font-size on the body element
font-size on the body element, however, does not impact how rem units are computed which means we get the desired base font size, and do not affect the spacing scale.
There was a problem hiding this comment.
Is it just my eyes or does that make the font size increase in the trace details?
There was a problem hiding this comment.
I see it too. @KenanYusuf do we need to make any other adjustments to retain the previous font size?
There was a problem hiding this comment.
Just to close the loop on this, Ryan and I are going to handle this in a different PR, so we can revert this change from this PR.
There was a problem hiding this comment.
🚨 Personal opinion warning 🚨
I'm not sure that these new tab designs make it clear enough that these are tabs, and that they hide additional information.
The sizing of the text for them is smaller than that of the (collapsible) sub-headings in the detail view, and even knowing that the tabs were there, I found myself having to "look for them".
Because visually, the selected tab is not too dissimilar to the tags that we use to reflect request method and status code, I think that the casual observer might think that these are other metadata of some kind, and I actually think the previous tabs design made it clearer that these are tabs.
@ryansrofe What do you think?
packages/webui/src/styles/base.css
Outdated
| @@ -5,7 +5,7 @@ | |||
| @tailwind utilities; | |||
| @tailwind variants; | |||
|
|
|||
| html { | |||
| body { | |||
There was a problem hiding this comment.
Fair enough. Not sure I can remember why I did this on html in the first place, but just out of curiosity can you share an example of what wasn't working correctly with this, and why the change to body fixes it... I feel like there's a lesson for me to learn here.
