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

feat: add stretchTabs to MatTabsConfig #26644

Merged
merged 6 commits into from
Feb 22, 2023
Merged

feat: add stretchTabs to MatTabsConfig #26644

merged 6 commits into from
Feb 22, 2023

Conversation

joelkesler
Copy link
Contributor

@joelkesler joelkesler commented Feb 18, 2023

PR for issue #26645


The new Material component updates changed how tabs present when used. Tabs now stretch to fill the header.

For those of us who wish to preserve the old behaviour, adding the stretchTabs settings to MatTabsConfig will be incredibly helpful to let us set the behaviour universally instead of updating each and every instance.

Ex:

providers: [
  {
    provide: MAT_TABS_CONFIG,
    useValue: {
      stretchTabs: false,
    },
  },
]

Cheers!

@angular-robot angular-robot bot added the detected: feature PR contains a feature commit label Feb 18, 2023
Copy link
Member

@crisbeto crisbeto left a comment

Choose a reason for hiding this comment

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

LGTM

@crisbeto crisbeto added action: merge The PR is ready for merge by the caretaker target: minor This PR is targeted for the next minor release merge: fix commit message When the PR is merged, rewrites/fixups of the commit messages are needed labels Feb 18, 2023
@@ -566,6 +566,10 @@ export class MatTabGroup extends _MatTabGroupBase {
defaultConfig && defaultConfig.fitInkBarToContent != null
? defaultConfig.fitInkBarToContent
: false;
this.stretchTabs =
Copy link
Member

Choose a reason for hiding this comment

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

This should default to true on the MDC-based tabs and false on the legacy ones.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for catching that. Updated

Copy link
Contributor Author

@joelkesler joelkesler Feb 19, 2023

Choose a reason for hiding this comment

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

I believe all I needed to do is change the default value from false to true as _stretchTabs is set to true earlier in the file here when there is no defaultConfig supplied, correct?

@crisbeto crisbeto removed the action: merge The PR is ready for merge by the caretaker label Feb 18, 2023
@crisbeto
Copy link
Member

@joelkesler the change is almost ready to go, but it needs to be rebased.

@joelkesler
Copy link
Contributor Author

Done ✅

@joelkesler
Copy link
Contributor Author

I also ran the recommended formatting command from the lint check:

yarn ng-dev format files src/material/tabs/tab-config.ts src/material/tabs/tab-group.ts

I am not sure what to do about the other failing checks. Let me know if there is more I can do!

@crisbeto
Copy link
Member

For the linting check you'd have to change the commit message to start with feat(material/tabs):. I can do that while merging though.

For the API golden failure you need to run yarn approve-api tabs and commit the result.

@joelkesler
Copy link
Contributor Author

For the API golden failure you need to run yarn approve-api tabs and commit the result.

Done ✅

@crisbeto crisbeto added action: merge The PR is ready for merge by the caretaker target: major This PR is targeted for the next major release and removed target: minor This PR is targeted for the next minor release labels Feb 22, 2023
@crisbeto crisbeto merged commit 506bca5 into angular:main Feb 22, 2023
@@ -566,6 +566,8 @@ export class MatTabGroup extends _MatTabGroupBase {
defaultConfig && defaultConfig.fitInkBarToContent != null
? defaultConfig.fitInkBarToContent
: false;
this.stretchTabs =
defaultConfig && defaultConfig.stretchTabs != null ? defaultConfig.stretchTabs : true;
Copy link

@liesahead liesahead Mar 8, 2023

Choose a reason for hiding this comment

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

Why not this? I'm just wondering.

defaultConfig?.stretchTabs ?? true

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For me, I was copying the way it was done in the file already, with my thinking that it was the quickest way to get the change approved and merged :)

@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 13, 2023
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 detected: feature PR contains a feature commit merge: fix commit message When the PR is merged, rewrites/fixups of the commit messages are needed target: major This PR is targeted for the next major release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants