Skip to content

Conversation

@zelliott
Copy link
Collaborator

@zelliott zelliott commented Mar 4, 2022

This PR updates MDC nav tabs to throw an error if the [tabPanel] input is not provided. Note that no such logic is added to the non-MDC tabs.

Additionally:

  • Update Material test cases to reflect that tab panel existing is the default case, while tab panel not existing is an exceptional/separate case.
  • Update MDC test cases to reflect that tab panel existing is the default case, and remove tests for tab panel not existing entirely (as we now throw an error if a tab panel isn't provided).
  • Updated MDC demos.
  • Updated README with new delta between Material and MDC tabs.
  • Fixed a minor bug in tabs (caught by unit tests) where an active and disabled tab would still have a tabindex of 0 (it should be -1).

@zelliott zelliott marked this pull request as ready for review March 4, 2022 15:18
@andrewseguin andrewseguin added P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent target: minor This PR is targeted for the next minor release labels Mar 4, 2022
@andrewseguin andrewseguin added the action: merge The PR is ready for merge by the caretaker label Mar 4, 2022
@josephperrott josephperrott removed the request for review from a team March 4, 2022 17:24
],
'mdc-tabs': [
// These tests are excluded because they are verifying behavior that is not supported in MDC.
'should have no explicit roles',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the feature or missing tests be ported over? The MDC tabs should have feature parity with the existing ones.

Copy link
Collaborator Author

@zelliott zelliott Mar 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR changes MDC tabs to not have feature parity with the existing tabs. In the existing tabs, the [tabPanel] input is optional. If it isn't provided, then the tabs fall back to default link/navigation behavior to avoid breaking existing instances in g3 that are impossible for me to migrate to use the [tabPanel]. In the new MDC tabs, we throw an error if the [tabPanel] input isn't provided. Thus, as users migrate from existing to MDC, if they're not already using the [tabPanel] input, they'll need to do so.

Given the [tabPanel] input is enforced in MDC tabs, we don't need to verify/test any of the link/navigation fallback behavior. That's why I explicitly didn't port over these tests.

}

override ngAfterViewInit() {
if (!this.tabPanel && (typeof ngDevMode === 'undefined' || ngDevMode)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: this won't catch the case where it starts off with a panel and it gets removed afterwards. Not sure if we need to worry about it.

Copy link
Collaborator Author

@zelliott zelliott Mar 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that's an exceptional case that we don't need to warn against, but I'm happy to move the check to either a setter or ngOnChanges if we feel otherwise.

@amysorto amysorto merged commit f42fee0 into angular:master Mar 10, 2022
forsti0506 pushed a commit to forsti0506/components that referenced this pull request Apr 3, 2022
…ot provided (angular#24517)

* feat(material/tabs): Throw error in MDC tabs if [tabPanel] is not provided.

* feat(material/tabs): Exclude some tests from MDC

* feat(material/tabs): Add tab panel to kitchen sinks
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Apr 10, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

action: merge The PR is ready for merge by the caretaker P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent target: minor This PR is targeted for the next minor release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants