Skip to content
This repository has been archived by the owner on Jan 13, 2025. It is now read-only.

feat: add feature targeting for styles to tab-related packages #4838

Merged
merged 2 commits into from
Jun 24, 2019

Conversation

crisbeto
Copy link
Collaborator

Adds support for style feature targeting to the tab, tab-bar, tab-indicator and tab-scroller packages.

@codecov-io
Copy link

codecov-io commented Jun 15, 2019

Codecov Report

Merging #4838 into master will increase coverage by 0.29%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4838      +/-   ##
==========================================
+ Coverage   98.97%   99.27%   +0.29%     
==========================================
  Files         130      130              
  Lines        6347     6347              
  Branches      821      821              
==========================================
+ Hits         6282     6301      +19     
+ Misses         64       45      -19     
  Partials        1        1
Impacted Files Coverage Δ
packages/mdc-checkbox/component.ts 96.87% <0%> (+1.04%) ⬆️
packages/mdc-tab/component.ts 98.36% <0%> (+3.27%) ⬆️
packages/mdc-base/component.ts 100% <0%> (+3.57%) ⬆️
packages/mdc-ripple/component.ts 100% <0%> (+3.77%) ⬆️
packages/mdc-auto-init/index.ts 100% <0%> (+4.16%) ⬆️
packages/mdc-ripple/util.ts 97.56% <0%> (+4.87%) ⬆️
packages/mdc-tabs/tab/component.ts 100% <0%> (+5%) ⬆️
packages/mdc-switch/component.ts 98.27% <0%> (+6.89%) ⬆️
packages/mdc-radio/component.ts 95.83% <0%> (+8.33%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e8afc42...91f0dc9. Read the comment docs.

@crisbeto crisbeto force-pushed the tab-feature-targeting branch 2 times, most recently from d2adc78 to 571aed2 Compare June 15, 2019 13:33
@crisbeto crisbeto marked this pull request as ready for review June 15, 2019 13:42
@crisbeto
Copy link
Collaborator Author

cc @mmalerba

packages/mdc-tab-bar/_mixins.scss Outdated Show resolved Hide resolved
packages/mdc-tab-indicator/_mixins.scss Outdated Show resolved Hide resolved
packages/mdc-tab-bar/_mixins.scss Outdated Show resolved Hide resolved
packages/mdc-tab-indicator/_mixins.scss Outdated Show resolved Hide resolved
packages/mdc-tab-indicator/_mixins.scss Outdated Show resolved Hide resolved
packages/mdc-tab/_mixins.scss Outdated Show resolved Hide resolved
@crisbeto crisbeto force-pushed the tab-feature-targeting branch 2 times, most recently from 6da9ed5 to b416918 Compare June 17, 2019 21:05
@crisbeto
Copy link
Collaborator Author

@abhiomkar @mmalerba I've reworked it based on the feedback.

@mmalerba
Copy link
Collaborator

Can you give a diff of the final CSS just to sanity check? Code looks good to me

@crisbeto
Copy link
Collaborator Author

Sure, here's the before and after. The main difference is that the order of a few selectors has changed, but from what I could tell it shouldn't have an effect on the end result. Also the after styles are a bit smaller, because the current ones in master include an extra license header in the middle of the file.

before.zip
after.zip

@mmalerba
Copy link
Collaborator

Most of the look pretty trivial, though mdc.tab.css has some significant movement (maybe that's finem just wanted to point it out: http://www.mergely.com/53ydf5dw/)

package-lock.json Outdated Show resolved Hide resolved
packages/mdc-tab-bar/_mixins.scss Show resolved Hide resolved
packages/mdc-tab-indicator/_mixins.scss Outdated Show resolved Hide resolved
packages/mdc-tab/_mixins.scss Outdated Show resolved Hide resolved
Adds support for style feature targeting to the `tab`, `tab-bar`, `tab-indicator` and `tab-scroller` packages.
@crisbeto crisbeto force-pushed the tab-feature-targeting branch from b416918 to 90a2b4a Compare June 19, 2019 16:01
@crisbeto
Copy link
Collaborator Author

I've reworked it based on the latest set of feedback: I got rid of the smaller private mixins and the ones that are left should have feature targets. Can you take another look @abhiomkar?

Copy link
Collaborator

@abhiomkar abhiomkar left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks @crisbeto!

@abhiomkar abhiomkar merged commit a47ef1d into material-components:master Jun 24, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants