Skip to content

feat: add stretchTabs to MatTabsConfig #26644

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.

Sign up for GitHub

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

Jump to bottom
Merged
crisbeto merged 6 commits into from
Feb 22, 2023
Merged

feat: add stretchTabs to MatTabsConfig #26644

crisbeto merged 6 commits into from
Feb 22, 2023

Conversation

@joelkesler
Copy link
Contributor

@joelkesler joelkesler commented Feb 18, 2023 •
edited
Loading

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
@joelkesler joelkesler marked this pull request as ready for review February 18, 2023 00:47
@joelkesler joelkesler requested a review from crisbeto as a code owner February 18, 2023 00:47
@joelkesler joelkesler mentioned this pull request Feb 18, 2023
Closed
crisbeto
crisbeto approved these changes Feb 18, 2023
View reviewed changes
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
crisbeto
crisbeto requested changes Feb 18, 2023
View reviewed changes
src/material/tabs/tab-group.ts
defaultConfig && defaultConfig.fitInkBarToContent != null
? defaultConfig.fitInkBarToContent
: false;
this.stretchTabs =
Copy link
Member

@crisbeto crisbeto Feb 18, 2023

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

@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.

Thanks for catching that. Updated

Copy link
Contributor Author

@joelkesler joelkesler Feb 19, 2023 •
edited
Loading

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
@joelkesler joelkesler requested a review from crisbeto February 20, 2023 01:23
@crisbeto
Copy link
Member

crisbeto commented Feb 20, 2023

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

@joelkesler
Copy link
Contributor Author

joelkesler commented Feb 21, 2023

Done ✅

@joelkesler
Copy link
Contributor Author

joelkesler commented Feb 21, 2023

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

crisbeto commented Feb 22, 2023

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

joelkesler commented Feb 22, 2023

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
@gregmagolan gregmagolan mentioned this pull request Mar 8, 2023
Closed
liesahead
liesahead reviewed Mar 8, 2023
View reviewed changes
src/material/tabs/tab-group.ts
? defaultConfig.fitInkBarToContent
: false;
this.stretchTabs =
defaultConfig && defaultConfig.stretchTabs != null ? defaultConfig.stretchTabs : true;
Copy link

@liesahead liesahead Mar 8, 2023 •
edited
Loading

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

@joelkesler joelkesler Mar 13, 2023

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

angular-automatic-lock-bot bot commented Apr 13, 2023

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.

Reviewers

@crisbeto crisbeto crisbeto approved these changes

@devversion devversion Awaiting requested review from devversion

@josephperrott josephperrott Awaiting requested review from josephperrott

+1 more reviewer

@liesahead liesahead liesahead left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@crisbeto crisbeto

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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@joelkesler @crisbeto @liesahead