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

fix(ripple): Use mdc-dom.matches everywhere instead of a custom one (#4340) #4372

Merged
merged 2 commits into from
Feb 14, 2019
Merged

fix(ripple): Use mdc-dom.matches everywhere instead of a custom one (#4340) #4372

merged 2 commits into from
Feb 14, 2019

Conversation

julien1619
Copy link
Contributor

@julien1619 julien1619 commented Feb 7, 2019

Fixes #4340

mdc-dom implements a matches() method that wasn't used everywhere. These custom implementations prevented some SSR usage of the material-components library. This commit removes all the custom implementations.

I removed some tests that were testing a now removed method, and I handled to make the other pass.

PS: This is my first contribution here, I hope I did everything right!

BREAKING CHANGE: getMatchesProperty() has been removed from @material/ripple/util. Use matches() from @material/dom/ponyfill instead.

…4340)

mdc-dom is implementing a `matches` method that wasn't used everywhere. These custom implementations
prevented some SSR usage of the material-components library. This commit removes all the custom
implementations.

#4340
@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

@julien1619
Copy link
Contributor Author

I've signed the CLA.

@googlebot
Copy link

CLAs look good, thanks!

@googlebot googlebot added cla: yes and removed cla: no labels Feb 7, 2019
@codecov-io
Copy link

codecov-io commented Feb 7, 2019

Codecov Report

Merging #4372 into master will increase coverage by 0.32%.
The diff coverage is 60%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4372      +/-   ##
==========================================
+ Coverage   98.56%   98.89%   +0.32%     
==========================================
  Files         130      130              
  Lines        5731     5714      -17     
  Branches      763      762       -1     
==========================================
+ Hits         5649     5651       +2     
+ Misses         82       63      -19
Impacted Files Coverage Δ
packages/mdc-tab-scroller/util.js 100% <ø> (ø) ⬆️
packages/mdc-ripple/util.js 100% <ø> (+3.5%) ⬆️
packages/mdc-switch/index.js 97.91% <0%> (+6.07%) ⬆️
packages/mdc-textfield/index.js 94.11% <0%> (-0.04%) ⬇️
packages/mdc-checkbox/index.js 100% <100%> (+1.2%) ⬆️
packages/mdc-ripple/index.js 100% <100%> (+2.08%) ⬆️
packages/mdc-tab-scroller/index.js 100% <100%> (ø) ⬆️
packages/mdc-auto-init/index.js 100% <0%> (+2.77%) ⬆️
packages/mdc-tab/index.js 96.49% <0%> (+3.5%) ⬆️
... and 5 more

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 3102351...eea2f61. Read the comment docs.

@kfranqueiro
Copy link
Contributor

Thanks for working on this! However, FYI, we're in the midst of converting all of our modules to TypeScript on the feat/typescript branch and we've already converted quite a few of these files. It might be worth waiting for all affected files to be converted and then applying these changes to the TypeScript modules instead... (in particular we haven't converted the tab packages yet)

Sorry for the inconvenience/timing!

@julien1619
Copy link
Contributor Author

No problem! I'll adapt my PR when it's ready. Do you know when you plan to merge the typescript version?

@kfranqueiro
Copy link
Contributor

We're aiming for merging the typescript branch into master sometime during March so that it'll be available in the minor release in April.

Copy link
Contributor

@acdvorak acdvorak 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 so much for the PR @julien1619!

@kfranqueiro
Copy link
Contributor

kfranqueiro commented Feb 14, 2019

To clarify, it turns out Andy already made equivalent changes to this on the TypeScript branch, so we're going to go ahead and take this PR for purposes of being able to resolve #4340 in a minor release (since it causes breaking changes by removing util APIs, technically) without waiting for the TypeScript conversion to land.

@acdvorak acdvorak merged commit 55e4229 into material-components:master Feb 14, 2019
acdvorak pushed a commit that referenced this pull request Feb 14, 2019
Fixes #4340

`mdc-dom` implements a `matches()` method that wasn't used everywhere. These custom implementations prevented some SSR usage of the material-components library. This commit removes all the custom implementations.

I removed some tests that were testing a now removed method, and I handled to make the other pass.

PS: This is my first contribution here, I hope I did everything right!

BREAKING CHANGE: `getMatchesProperty()` has been removed from `@material/ripple/util` and `@material/tab-scroller/util`. Use `matches()` from `@material/dom/ponyfill` instead.
@julien1619
Copy link
Contributor Author

Great! Can't wait to see the next release ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants