Skip to content

fix(material-experimental/mdc-tabs): correct min-width according to m… - #20979

Merged
annieyw merged 1 commit into
masterfrom
mdc-tabs-min-width
Jan 9, 2021
Merged

fix(material-experimental/mdc-tabs): correct min-width according to m…#20979
annieyw merged 1 commit into
masterfrom
mdc-tabs-min-width

Conversation

@wagnermaciel

@wagnermaciel wagnermaciel commented Nov 5, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor

…dc specs

Link to the specs in question: https://material.io/components/tabs#specs

@google-cla google-cla Bot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Nov 5, 2020
crisbeto
crisbeto approved these changes Nov 6, 2020
View reviewed changes

@crisbeto crisbeto left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM. For the record, I used the old min-width from the non-MDC version to keep the two closer visually.

@crisbeto crisbeto added action: merge The PR is ready for merge by the caretaker target: patch This PR is targeted for the next patch release labels Nov 6, 2020
jelbourn
jelbourn reviewed Nov 6, 2020
View reviewed changes
Comment thread src/material-experimental/mdc-tabs/_tabs-common.scss Outdated
// we need the extra specificity to avoid issues with CSS insertion order.
flex-grow: 0;
min-width: 160px;
min-width: 90px;

@jelbourn jelbourn Nov 6, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If the min-width is in the spec, it seems like either MDC should set it directly. Could you check with the MDC folks about whether the omission of this from their styles is intentional?

I know this is really nitpicky for one style, but the goal of us basing our components on top of MDC is to converge as much as possible. Every extra line of style we add on top is a step away from that goal.

@wagnermaciel wagnermaciel Jan 6, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

MDC now exposes min-width (see commit). Once we bump our canary version (see #21501) we can just delete our custom min-width and use the default one.

@wagnermaciel wagnermaciel mentioned this pull request Nov 6, 2020
Closed
@wagnermaciel wagnermaciel removed the action: merge The PR is ready for merge by the caretaker label Nov 17, 2020
@wagnermaciel wagnermaciel added the action: merge The PR is ready for merge by the caretaker label Jan 6, 2021
@mmalerba

mmalerba commented Jan 6, 2021

Copy link
Copy Markdown
Contributor

Blocked on #21501

@mmalerba mmalerba added blocked This issue is blocked by some external factor, such as a prerequisite PR P2 The issue is important to a large percentage of users, with a workaround labels Jan 6, 2021
@annieyw annieyw added target: minor This PR is targeted for the next minor release and removed target: patch This PR is targeted for the next patch release blocked This issue is blocked by some external factor, such as a prerequisite PR labels Jan 7, 2021
@annieyw
annieyw merged commit c266f3b into master Jan 9, 2021
@wagnermaciel
wagnermaciel deleted the mdc-tabs-min-width branch January 13, 2021 20:41
wagnermaciel added a commit to wagnermaciel/components that referenced this pull request Jan 14, 2021
@wagnermaciel
fix(material-experimental/mdc-tabs): delete custom tab min-width (ang…
11bf865 
…ular#20979)
@angular-automatic-lock-bot

angular-automatic-lock-bot Bot commented Feb 13, 2021

Copy link
Copy Markdown

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 Feb 13, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@crisbeto crisbeto crisbeto approved these changes
+1 more reviewer
@jelbourn jelbourn jelbourn left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

action: merge The PR is ready for merge by the caretaker cla: yes PR author has agreed to Google's Contributor License Agreement P2 The issue is important to a large percentage of users, with a workaround target: minor This PR is targeted for the next minor release

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@wagnermaciel @mmalerba @jelbourn @crisbeto @annieyw