Skip to content
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

[Bug]: Tab CSS breaks backwards compatibility and multiple usages of Layer component #17096

Closed
2 tasks done
wkeese opened this issue Aug 2, 2024 · 2 comments · Fixed by #17215
Closed
2 tasks done
Assignees

Comments

@wkeese
Copy link
Contributor

wkeese commented Aug 2, 2024

Package

@carbon/react

Browser

Chrome

Package version

1.61.0

React version

18

Description

PR #16738 broke the styling (background colors) for all our tabs, and also breaks the usage of <Layer> within tab panels.

It's due to this SCSS below:

.#{$prefix}--tabs--contained ~ .#{$prefix}--tab-content {
    background: $layer;

    > * {
      @include layer-tokens.emit-layer-tokens(2);
    }
  }

This CSS is conceptually similar to what we’ve been doing to declare tiles etc. which is essentially:

<div style="background: var(--cds-layer)">
    <Layer></Layer>
</div>

However, the CSS from #16738 doesn’t quite achieve that for a couple of reasons:

  1. It breaks markup where the first DOM child is a <Layer>, for example:

    <TabPanel>
       <Layer level={2}></Layer>
    </TabPanel>

    The CSS above overrides the tokens defined by the<Layer>, so the <Layer> component is essentially ignored.

  2. Nested <Layer> components don’t work as expected either. For example, both these inputs appear the same, even though the <Layer> is supposed to give the second input a different background:

     <TabPanel>
         <fieldset>
             <TextInput id="one" labelText="plain"/>
             <Layer>
               <TextInput id="two" labelText="with layer"/>
             </Layer>
         </fieldset>
     </TabPanel>

    It's because although you defined the CSS tokens for level 2, the Layer context is still on level 1. They are out of sync.

  3. It breaks backward compatibility with 1.60.0 (although perhaps this was intentional?), as code like below will show up with a different background color than previously:

    <TabPanel>
       <div style={{background: "var(--cds-layer)"}}>...</div>
    </TabPanel>

If you want TabPanel to set up a Layer context, I think the right way to implement this would be for TabPanel to actually embed a <Layer> tag. Conceptually I think that makes sense but:

  1. That would continue to break backward compatibility, see item 3 above.
  2. If TabPanel sets a background color and embeds a <Layer> component, then shouldn't other components like Modal, Tile, etc. do the same?

If anyone is hitting this problem and looking for a quick imperfect workaround, this is what we did:

@use "@carbon/styles/scss/layer/layer-tokens" as layer-tokens;
...
.cds--tabs--contained ~ .cds--tab-content > * {
  @include layer-tokens.emit-layer-tokens(1);
}

Reproduction/example

https://stackblitz.com/edit/github-ihxqde?file=src%2FApp.jsx

Steps to reproduce

Just look at the stackblitz.

Suggested Severity

None

Application/PAL

IKC

Code of Conduct

@finken2
Copy link
Contributor

finken2 commented Aug 8, 2024

Following. We're running into similar layer issues within tabs trying to move up from @carbon/react 1.53.1 to 1.62.1.
Here's a simple example in screenshots. Note the search bar and empty state. It gets worse if an actual table is shown.

Before (1.53.1):
image

After (1.62.1):
image

@vishalpurandare
Copy link

Is there an ETA for the fix on this one? thank you.

@github-project-automation github-project-automation bot moved this from 🏗 In Progress to ✅ Done in Design System Aug 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

6 participants