material-components / material-components-web

Modular and customizable Material Design UI components for the web
https://material.io/develop/web
MIT License
17.15k stars 2.15k forks source link

[TabBar] MDCTabBar: At least one tab must have mdc-tab--active class #4226

Closed RobJacobs closed 5 years ago

RobJacobs commented 5 years ago

There must be one tab element in the tab bar component with a class of mdc-tab--active or an error is thrown in the console: Cannot read property 'deactivate' of undefined when switching tabs

What MDC Web Version are you using?

Latest from master branch.

What browser(s) is this bug affecting?

All

What are the steps to reproduce the bug?

See this CodePen example. Notice the error in the console that results from trying to use the tab bar activateTab method. Notice the error is also thrown when clicking on tabs. Uncommenting the last tab in the example fixes the issue as it has a mdc-tab--active class.

What is the expected behavior?

Using the mdc-tab--active class to initially set the active tab should not be required. You should be able to use the mdc-tab activateTab method to set the initial active tab.

What is the actual behavior?

You must use the mdc-tab--active on at least one child mdc-tab element.

abhiomkar commented 5 years ago

Thanks for reporting this issue.

This is expected behaviour of MDC TabBar, it mandates to have at least one tab to be active by default. Please refer Tabs spec article on Material Design site.

Can you please give us some more context on why would the tabs won't be active by default in your example?

RobJacobs commented 5 years ago

I'm setting the active tab in my javascipt code using the tab bar activateTab method. I was hoping I could avoid having to add the mdc-tab--active class in my markup and just rely on that method instead. I updated the original comment to better reflect the issue.

I changed the following lines to make it work:

foundation

if (previousActiveIndex !== -1) {
  this.adapter_.deactivateTabAtIndex(previousActiveIndex);
}

index

getTabIndicatorClientRectAtIndex: (index) => { if (this.tabList_[index]) { this.tabList_[index].computeIndicatorClientRect(); } },
acdvorak commented 5 years ago

Seems like a valid use case to me. Thanks for the suggested fixes, @RobJacobs!

I think we might want to change this:

https://github.com/material-components/material-components-web/blob/e718cb2ba8c33fa9b8af5b5d73097db9c02b1a07/packages/mdc-tab-bar/foundation.js#L121-L132

to this:

  activateTab(index) {
    const previousActiveIndex = this.adapter_.getPreviousActiveTabIndex();
    if (!this.indexIsInRange_(index) || index === previousActiveIndex) {
      return;
    }

    if (previousActiveIndex > -1) {
      this.adapter_.deactivateTabAtIndex(previousActiveIndex);
      this.adapter_.activateTabAtIndex(index, this.adapter_.getTabIndicatorClientRectAtIndex(previousActiveIndex));
    } else {
      this.adapter_.activateTabAtIndex(index, this.adapter_.getTabIndicatorClientRectAtIndex(index));
    }

    this.scrollIntoView(index);

    this.adapter_.notifyTabActivated(index);
  }

However, I'll have to check with @patrickrodee to make sure that passing index to getTabIndicatorClientRectAtIndex() won't break anything.

We'll also need to add unit test(s) for it, of course.

acdvorak commented 5 years ago

After talking to Patty, the entrance animation is going to look weird with the changes I suggested above, because we would be animating from coordinate (0, 0) to wherever the new active tab is.

@RobJacobs Can you tell us a little bit more about your use case? Why aren't you able to add the --active CSS classes in the HTML?

RobJacobs commented 5 years ago

@acdvorak Thanks for looking into this. When a user loads the view with the tab-bar, I want to set the active tab to the tab they were previously on. Using the activateTab method seemed like the better approach rather than having to write my own querySelector or go after children[index] to apply the --active classes (both the tab and tab-indicator) manually. Could you bypass the animation if there is no previously activated tab?

kfranqueiro commented 5 years ago

Thanks for the explanation. The idea seems reasonable; we'll need to see what implementation details are required to bypass the animation in that case.

marcoherzog commented 5 years ago

I am facing the same issue as I am implementing multiple MDCTabBars as navigation elements on a theme. Indicating the current page. The navigation on top right corner does not have an active item. Sure... might not be the original use case.

Screenshot 2019-03-16 at 11 26 10