fix(tab-title): tabs center when set to `layout='center'` #7026

Elijbet · 2023-05-24T23:59:13Z

Related Issue: #7025

Summary

Enhancement to tab-title to make it optionally closable introduced structural changes that caused a regression in the tab alignment when centered.

…er-layout

alisonailea · 2023-05-25T00:57:12Z

src/components/tab-title/tab-title.scss

@@ -7,7 +7,9 @@
 :host([layout="center"]) {
  @apply my-0 mx-5 text-center;
  flex-basis: theme("spacing.48");
-  margin: auto;
+  .content {
+    @apply m-auto;


given that there's no difference between margin: auto and @apply m-auto I'd recommend using margin: auto it's more readable

Since we're using tailwind for now, it makes sense to stick to tailwind classes if they are available, for consistency. This is what we've been doing so far. Also, overlapping uses of tailwind and regular css props might create oversight doubling use of both by accident.

Agreed, lets keep tailwind for now for consistency. Hopefully we can remove tailwind in the future.

macandcheese · 2023-05-25T02:58:01Z

Can we add some additional screenshot tests for some more specific pre-configured cases like centered + closable, closable + bordered + centered, to hopefully catch some of these?

driskull

👍

…edClosable

src/components/tabs/tabs.stories.ts

…er-layout

Elijbet · 2023-05-25T20:12:58Z

Can we add some additional screenshot tests for some more specific pre-configured cases like centered + closable, closable + bordered + centered, to hopefully catch some of these?

Adding these tests revealed some mismatch with the centering with regards to Figma. There are various ways to center text/icon and the x button within a tab, as discussed with @macandcheese.

We were wondering what to prioritize: matching the Figma design to make it absolutely centered (but with variable padding to offset for added close button width), or making sure the text/icon is centered between the edge of the tab and the close button. I've applied the latter for your review @SkyeSeitz @ashetland

driskull

🦭 Seal of approval! ☑️ 👍 💯 👏

driskull · 2023-05-25T21:55:28Z

src/components/tab-title/tab-title.scss

@@ -7,7 +7,9 @@
 :host([layout="center"]) {
  @apply my-0 mx-5 text-center;
  flex-basis: theme("spacing.48");
-  margin: auto;
+  .content {
+    @apply m-auto;


Agreed, lets keep tailwind for now for consistency. Hopefully we can remove tailwind in the future.

driskull · 2023-05-25T21:56:05Z

src/components/tab-title/tab-title.scss

@@ -190,11 +192,21 @@
  }
 }

+:host([closable]) {
+  .content {
+    padding-inline-start: 1.5rem;


@alisonailea @Elijbet are there any tokens we can use as values for these paddings?f

If this padding should be referenced from theme()/tailwind we could do it as a followup.

Let me see what design says about the centering approach in general (will determine the padding amount) and yeah that would be the next question.

…er-layout

SkyeSeitz · 2023-05-26T00:04:53Z

Let's prioritize absolutely centering the Tab Titles with variable padding to offset for added close button width.

small x-button offset: 24px; core.sizing.9
medium x-button offset: 24px; core.sizing.9
large x-button offset: 32px; core.sizing.11

Secondly, I would propose truncating the text when it reaches 8px away from the x-button at all scales. Not sure if this is in scope for this particular issue, though, but this is a better behavior than the current wrapping.

…oid moving when on or off

Elijbet · 2023-05-26T00:34:19Z

Secondly, I would propose truncating the text when it reaches 8px away from the x-button at all scales. Not sure if this is in scope for this particular issue, though, but this is a better behavior than the current wrapping.

I'll open a separate issue for the truncation, as it would need to have the tooltip applied for sighted users, just like the truncated button does.

…er-layout

…ext, fix the text moving upwards in the non-bordered centered tab

Elijbet · 2023-05-26T07:03:22Z

src/components/tab-title/tab-title.scss

+:host([layout="center"][closable]) {
+  .content {
+    padding-inline-start: 32px; //28px button width + 0.25rem padding
+  }


Can I get guidance on how to implement tokens here?
eg. padding-inline-start: var(--core-sizing-11);
This doesn't seem to take any effect.

Or is it something we could do as a follow-up like you said earlier @driskull?

@alisonailea @jcfranco

@Elijbet this is good for the release. Can you open an issue to address this by using our design tokens/tailwind for this padding? Ideally both the button width and padding should be tied to variables.

…he width of `tab-nav` (#7047) **Related Issue:** #7025 ## Summary Follow-up to fix to #7026. Centered `tabs` should spread the `tab-titles` evenly to extend across the entire width of `tab-nav`, as opposed to `inline tab-titles` that only justify against the start of `tab-nav` width. Added coverage for both `centeredTabsAreEvenlyJustifiedAcrossNavWidth` and `inlineTabsJustifyAgainstTheStartOfTheNavWidth`.

fix(tab-title): tabs center when set to layout='center'

3d9bb3b

github-actions bot added the bug Bug reports for broken functionality. Issues should include a reproduction of the bug. label May 24, 2023

github-actions bot assigned Elijbet May 24, 2023

Merge branch 'master' into elijbet/7025-fix-tab-title-regression-cent…

9b1f151

…er-layout

Elijbet marked this pull request as ready for review May 25, 2023 00:16

Elijbet requested a review from a team as a code owner May 25, 2023 00:16

Elijbet added the pr ready for visual snapshots Adding this label will run visual snapshot testing. label May 25, 2023

alisonailea reviewed May 25, 2023

View reviewed changes

alisonailea approved these changes May 25, 2023

View reviewed changes

driskull approved these changes May 25, 2023

View reviewed changes

additional preconfigured stories: centeredClosable and centeredBorder…

11758a0

…edClosable

Elijbet added pr ready for visual snapshots Adding this label will run visual snapshot testing. and removed pr ready for visual snapshots Adding this label will run visual snapshot testing. labels May 25, 2023

add more variations on tab-title to catch andy possible padding issues

18f63c0

jcfranco added pr ready for visual snapshots Adding this label will run visual snapshot testing. and removed pr ready for visual snapshots Adding this label will run visual snapshot testing. labels May 25, 2023

macandcheese reviewed May 25, 2023

View reviewed changes

src/components/tabs/tabs.stories.ts Outdated Show resolved Hide resolved

Elijbet added 2 commits May 25, 2023 13:05

centering tab-title and close button within the tab

92966a4

Merge branch 'master' into elijbet/7025-fix-tab-title-regression-cent…

4664adb

…er-layout

Elijbet added pr ready for visual snapshots Adding this label will run visual snapshot testing. and removed pr ready for visual snapshots Adding this label will run visual snapshot testing. labels May 25, 2023

driskull approved these changes May 25, 2023

View reviewed changes

Merge branch 'master' into elijbet/7025-fix-tab-title-regression-cent…

d0b914d

…er-layout

center the text visually and not affected by the x button so as to av…

7c36006

…oid moving when on or off

Elijbet added pr ready for visual snapshots Adding this label will run visual snapshot testing. and removed pr ready for visual snapshots Adding this label will run visual snapshot testing. labels May 26, 2023

Elijbet added 2 commits May 25, 2023 17:42

apply to layout center only

400201a

Merge branch 'master' into elijbet/7025-fix-tab-title-regression-cent…

006dff3

…er-layout

Elijbet added pr ready for visual snapshots Adding this label will run visual snapshot testing. and removed pr ready for visual snapshots Adding this label will run visual snapshot testing. labels May 26, 2023

Elijbet added 2 commits May 25, 2023 18:30

Merge branch 'master' into elijbet/7025-fix-tab-title-regression-cent…

704ff62

…er-layout

ensure correct offset so adding/removing x button does not move the t…

de47d0f

…ext, fix the text moving upwards in the non-bordered centered tab

Elijbet added pr ready for visual snapshots Adding this label will run visual snapshot testing. and removed pr ready for visual snapshots Adding this label will run visual snapshot testing. labels May 26, 2023

fill in missing scale s bordered closable centered

e2d3a7b

Elijbet added pr ready for visual snapshots Adding this label will run visual snapshot testing. and removed pr ready for visual snapshots Adding this label will run visual snapshot testing. labels May 26, 2023

Elijbet commented May 26, 2023

View reviewed changes

Elijbet merged commit 0bd677b into master May 26, 2023

Elijbet deleted the elijbet/7025-fix-tab-title-regression-center-layout branch May 26, 2023 15:43

github-actions bot added this to the 1.4.1 May patch - Bugs only milestone May 26, 2023

Elijbet mentioned this pull request May 26, 2023

fix(tab-nav): justify centered tab-titles evenly to extend across the width of tab-nav #7047

Merged

Elijbet mentioned this pull request May 31, 2023

[Tab-Title]: truncate text past certain length #7068

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(tab-title): tabs center when set to `layout='center'` #7026

fix(tab-title): tabs center when set to `layout='center'` #7026

Elijbet commented May 24, 2023 •

edited by jcfranco

Loading

alisonailea May 25, 2023

Elijbet May 25, 2023

driskull May 25, 2023

macandcheese commented May 25, 2023 •

edited

Loading

driskull left a comment

Elijbet commented May 25, 2023 •

edited

Loading

driskull left a comment

driskull May 25, 2023

driskull May 25, 2023

driskull May 25, 2023

Elijbet May 25, 2023 •

edited

Loading

SkyeSeitz commented May 26, 2023 •

edited

Loading

Elijbet commented May 26, 2023 •

edited

Loading

Elijbet May 26, 2023 •

edited

Loading

driskull May 26, 2023

fix(tab-title): tabs center when set to layout='center' #7026

fix(tab-title): tabs center when set to layout='center' #7026

Conversation

Elijbet commented May 24, 2023 • edited by jcfranco Loading

Summary

alisonailea May 25, 2023

Choose a reason for hiding this comment

Elijbet May 25, 2023

Choose a reason for hiding this comment

driskull May 25, 2023

Choose a reason for hiding this comment

macandcheese commented May 25, 2023 • edited Loading

driskull left a comment

Choose a reason for hiding this comment

Elijbet commented May 25, 2023 • edited Loading

driskull left a comment

Choose a reason for hiding this comment

driskull May 25, 2023

Choose a reason for hiding this comment

driskull May 25, 2023

Choose a reason for hiding this comment

driskull May 25, 2023

Choose a reason for hiding this comment

Elijbet May 25, 2023 • edited Loading

Choose a reason for hiding this comment

SkyeSeitz commented May 26, 2023 • edited Loading

Elijbet commented May 26, 2023 • edited Loading

Elijbet May 26, 2023 • edited Loading

Choose a reason for hiding this comment

driskull May 26, 2023

Choose a reason for hiding this comment

fix(tab-title): tabs center when set to `layout='center'` #7026

fix(tab-title): tabs center when set to `layout='center'` #7026

Elijbet commented May 24, 2023 •

edited by jcfranco

Loading

macandcheese commented May 25, 2023 •

edited

Loading

Elijbet commented May 25, 2023 •

edited

Loading

Elijbet May 25, 2023 •

edited

Loading

SkyeSeitz commented May 26, 2023 •

edited

Loading

Elijbet commented May 26, 2023 •

edited

Loading

Elijbet May 26, 2023 •

edited

Loading