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

Add "active-tab-index" attribute support for Tabs? #5297

Closed
datvm opened this issue Dec 14, 2023 · 3 comments · Fixed by #5456 or #5429 · May be fixed by X-oss-byte/pigweed#61, X-oss-byte/pigweed#70 or X-oss-byte/pigweed#73
Closed

Add "active-tab-index" attribute support for Tabs? #5297

datvm opened this issue Dec 14, 2023 · 3 comments · Fixed by #5456 or #5429 · May be fixed by X-oss-byte/pigweed#61, X-oss-byte/pigweed#70 or X-oss-byte/pigweed#73
Labels
Type: Feature New feature or request

Comments

@datvm
Copy link
Contributor

datvm commented Dec 14, 2023

Description

I think Lit Playground should work for <md-tabs> similar to <md-select>'s selected-index:

  <md-tabs active-tab-index="1">
    <md-primary-tab>Tab 1</md-primary-tab>
    <md-primary-tab>Tab 2</md-primary-tab>
  </md-tabs>

This would make life much easier for a lot of Web frameworks where it's much harder to access DOM property than HTML attributes.

@asyncLiz
Copy link
Collaborator

I think the request is reasonable, but are there pain points to setting the active attribute on the selected tab? In general that's where the source of truth should be.

<md-tabs>
  <md-primary-tab>Tab 1</md-primary-tab>
  <md-primary-tab active>Tab 2</md-primary-tab>
</md-tabs>

The parent's active tab index is more expensive than its children, since it will look up and set the child's attribute. Select is the same way, and discourages using selected-index for things like SSR.

@datvm
Copy link
Contributor Author

datvm commented Dec 15, 2023

Thanks for the suggestion, I can't recall now what issue I ran into trying to use active on individual tabs but at least one problem is that there is no animation when using it Lit Playground:

  <md-tabs active-tab-index="1">
    <md-primary-tab>Tab 1</md-primary-tab>
    <md-primary-tab active>Tab 2</md-primary-tab>
  </md-tabs>
  
  <p>
    <button>Switch Tab</button>
  </p>
  
  <script>
    const tabsEl = document.querySelector("md-tabs");
    const tabs = [...document.querySelectorAll("md-primary-tab")];
    
    document.querySelector("button").addEventListener("click", () => {
      const currTab = tabsEl.activeTabIndex;
      tabs[1 - currTab].setAttribute("active", "");
      tabs[currTab].removeAttribute("active");
    });
  </script>

Also in reactive frameworks, usually it's a bit awkward to use active since we need to keep track of the index in a for loop like this (pseudo-code) and then have a separate binding for onTabChanged:

@{
    var counter = 0;
}

<md-tabs @onchange="ProcessTabIndexChange">
    @foreach (var tab of tabs)
    {
        var z = counter++; // Must have a scoped variable here because counter changes
        <md-tab label="@(tab.Title)" active="@(z == currTabIndex)"></md-tab>
    }
</md-tabs>

If active-tab-index is available, it can become a bindable property, the above code can be simplified to this:

<md-tabs @bind-ActiveTabIndex="currTabIndex">
    @foreach (var tab of tabs)
    {
        <md-tab label="@(tab.Title)"></md-tab>
    }
</md-tabs>

@asyncLiz asyncLiz added the Type: Feature New feature or request label Jan 2, 2024
@adrianjost
Copy link
Contributor

adrianjost commented Feb 12, 2024

I agree and expected this behaviour based on the properties defined in the docs.

image

I'm using the bundled version from npm and was able to get it running by just adding the following patch:

diff --git a/node_modules/@material/web/tabs/internal/tabs.js b/node_modules/@material/web/tabs/internal/tabs.js
index 801d980..a2844e6 100644
--- a/node_modules/@material/web/tabs/internal/tabs.js
+++ b/node_modules/@material/web/tabs/internal/tabs.js
@@ -270,6 +270,9 @@ export class Tabs extends LitElement {
 __decorate([
     queryAssignedElements({ flatten: true, selector: '[md-tab]' })
 ], Tabs.prototype, "tabs", void 0);
+__decorate([
+    property({ type: Number, attribute: 'active-tab-index' })
+], Tabs.prototype, "activeTabIndex", void 0);
 __decorate([
     property({ type: Boolean, attribute: 'auto-activate' })
 ], Tabs.prototype, "autoActivate", void 0);

It looks like everything that is necessary to get it working is already there, there is just a binding to the outer world missing.

For the active prop on the tab elements themself, there exists a @property({type: Boolean, reflect: true}) active = false;.
I'm just getting started learning lit, but I would expect a similar @property({type: Number, reflect: true}) added to set activeTabIndex(index: number) to do just this.
Hadn't had time to test they yet though, because I didn't have this project setup yet.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment