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

feat(text-field): add feature targeting for styles #5378

Merged

Conversation

crisbeto
Copy link
Collaborator

@crisbeto crisbeto commented Dec 22, 2019

Sets up feature targeting for the text field and its related styles.

Relates to #4227.

cc @mmalerba @devversion

@crisbeto
Copy link
Collaborator Author

Here's the before/after output:
before.css.txt
after.css.txt

You'll notice that there are some differences. They come from the fact that I had to split the ripple styles into a separate mixin so that we can opt out of them for Angular Material. A side-effect of the split is that I also had to split away the outlined styles, because they need to be included after the ripple styles, because they have some overrides that hide the ripple. I've gone through all the examples in the screenshot test app and these changes don't seem to have an effect on the appearance.

@codecov-io
Copy link

codecov-io commented Dec 22, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@e3e425e). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #5378   +/-   ##
=========================================
  Coverage          ?   94.91%           
=========================================
  Files             ?      163           
  Lines             ?     6293           
  Branches          ?      788           
=========================================
  Hits              ?     5973           
  Misses            ?      320           
  Partials          ?        0

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 e3e425e...f246f22. Read the comment docs.

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.

Sent my first round of review comments.

There is incoming PR #5367 which is expected to merge today. You might want to rebase your branch after it is merged.

There are no more changes related to text field are expected this week.

Thanks!

packages/mdc-textfield/_mixins.scss Outdated Show resolved Hide resolved
@abhiomkar
Copy link
Collaborator

PR #5367 is merged. Can you please rebase with master, I'll test this PR internally today.

Thanks!

@crisbeto crisbeto force-pushed the text-field-feature-targeting branch from 1378560 to ce0be4d Compare January 10, 2020 20:59
@crisbeto
Copy link
Collaborator Author

@abhiomkar I've rebased it and reworked it based on the feedback. Here are the updated before/after files.

before.css.txt
after.css.txt

@crisbeto crisbeto force-pushed the text-field-feature-targeting branch from ce0be4d to 7ff5d1e Compare January 10, 2020 21:14
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.

Ready to test internally.

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.

Sent 3 minor review comments after manually testing this PR internally.

packages/mdc-textfield/_mixins.scss Show resolved Hide resolved
packages/mdc-textfield/_mixins.scss Outdated Show resolved Hide resolved
packages/mdc-textfield/_mixins.scss Outdated Show resolved Hide resolved
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.

This change is also breaking leading icon label position. Investigating the issue.

@abhiomkar
Copy link
Collaborator

Apart from above review comments, rest of the changes looks good!

This PR is conflicting with incoming change related to sass module refactor PR #5453

I'm not able to run global test since it is conflicting with above PR.

@jathak Let me know if you are able to get the sass module change in today or next week first-half. If you think you'll need some more time we can merge this first and re-run migrator script on top of this.

@jathak
Copy link
Contributor

jathak commented Jan 11, 2020

I think we can get the module system migration in by EOD Monday. I'm waiting on a TGP at 6 PM just to confirm that everything's working (a previous TGP with mostly similar code passed yesterday), but other than that I just need review/approval.

@crisbeto crisbeto force-pushed the text-field-feature-targeting branch from 7ff5d1e to 8b87d3e Compare January 11, 2020 11:39
@crisbeto
Copy link
Collaborator Author

crisbeto commented Jan 11, 2020

Thank you for looking into it @abhiomkar, I've pushed all the changes. It also cut the number of changes in the final output in half 👍.

@abhiomkar
Copy link
Collaborator

Thanks @crisbeto for prompt changes! As per above comment, we would wait till PR #5453 is merged. Once it is merged, you'll need to rebase your branch and resolve merge conflicts.

@jathak Sounds good.. We'll target to merge the Sass module system changes first.

@abhiomkar
Copy link
Collaborator

@asyncLiz Are we expecting any big changes to text field sass files? Can this PR go in first?

@asyncLiz
Copy link
Contributor

Nothing coming up. The only change we have soon for textfield is #5439, and that's just DOM structure changes.

@abhiomkar
Copy link
Collaborator

Thanks Liz!

@crisbeto It's time to rebase your PR with master. I'll run tests internally again once your PR is ready. Thanks!

@crisbeto crisbeto force-pushed the text-field-feature-targeting branch from 8b87d3e to d6d6f5d Compare January 16, 2020 23:00
@crisbeto
Copy link
Collaborator Author

I've resolved all of the conflicts and I have the feature targeting test passing, but the production build throws this error and I'm not quite sure how to deal with it:

 ERROR in ./packages/material-components-web/material-components-web.scss
    Module build failed (from ./node_modules/mini-css-extract-plugin/dist/loader.js):
    ModuleBuildError: Module build failed (from ./node_modules/sass-loader/lib/loader.js):
    undefined
    ^
          Module packages\material-components-web\node_modules\@material\animation\_variables.scss and the new module both forward a variable named $mdc-animation-deceleration-curve-timing-function.
       ╷
    42 │ @forward "mdc-text-field";
       │ ^^^^^^^^^^^^^^^^^^^^^^^^^
       ╵
      packages\material-components-web\node_modules\@material\textfield\mdc-text-field.import.scss 42:1  @import
      stdin 54:9                                                                                         root stylesheet
          in C:\projects\material-components-web\packages\material-components-web\node_modules\@material\textfield\mdc-text-field.import.scss (line 42, column 1)
        at runLoaders (C:\projects\material-components-web\node_modules\webpack\lib\NormalModule.js:302:20)
        at C:\projects\material-components-web\node_modules\loader-runner\lib\LoaderRunner.js:367:11
        at C:\projects\material-components-web\node_modules\loader-runner\lib\LoaderRunner.js:233:18
        at context.callback (C:\projects\material-components-web\node_modules\loader-runner\lib\LoaderRunner.js:111:13)
        at render (C:\projects\material-components-web\node_modules\sass-loader\lib\loader.js:52:13)
        at Function.call$2 (C:\projects\material-components-web\node_modules\dart-sass\sass.dart.js:55025:16)
        at _render_closure.call$0 (C:\projects\material-components-web\node_modules\dart-sass\sass.dart.js:33874:23)
        at Object.Primitives_applyFunction (C:\projects\material-components-web\node_modules\dart-sass\sass.dart.js:1057:30)
        at Object.Function_apply (C:\projects\material-components-web\node_modules\dart-sass\sass.dart.js:4894:16)
        at _callDartFunctionFast (C:\projects\material-components-web\node_modules\dart-sass\sass.dart.js:6574:16)
        at C:\projects\material-components-web\node_modules\dart-sass\sass.dart.js:6552:18
    Child mini-css-extract-plugin node_modules/css-loader/dist/cjs.js??ref--4-1!node_modules/postcss-loader/src/index.js??ref--4-2!node_modules/sass-loader/lib/loader.js??ref--4-3!packages/material-components-web/material-components-web.scss:
           1 module
        ERROR in ./packages/material-components-web/material-components-web.scss (./node_modules/css-loader/dist/cjs.js??ref--4-1!./node_modules/postcss-loader/src??ref--4-2!./node_modules/sass-loader/lib/loader.js??ref--4-3!./packages/material-components-web/material-components-web.scss)
        Module build failed (from ./node_modules/sass-loader/lib/loader.js):
        undefined
        ^
              Module packages\material-components-web\node_modules\@material\animation\_variables.scss and the new module both forward a variable named $mdc-animation-deceleration-curve-timing-function.
           ╷
        42 │ @forward "mdc-text-field";
           │ ^^^^^^^^^^^^^^^^^^^^^^^^^
           ╵
          packages\material-components-web\node_modules\@material\textfield\mdc-text-field.import.scss 42:1  @import
          stdin 54:9                                                                                         root stylesheet
              in C:\projects\material-components-web\packages\material-components-web\node_modules\@material\textfield\mdc-text-field.import.scss (line 42, column 1)

@abhiomkar
Copy link
Collaborator

Make sure you're testing with Sass version v1.24.3 or above. Looking at the above error message, it seems like it is using previous Sass version which had this known issue.

@crisbeto crisbeto force-pushed the text-field-feature-targeting branch from d6d6f5d to 36985dd Compare January 17, 2020 07:03
@crisbeto
Copy link
Collaborator Author

It was because I was mixing @use and @import in one of the files. It should be good to go now. Here are the before/after files:

before.css.txt
after.css.txt

@abhiomkar
Copy link
Collaborator

Seems like there are still few minor CSS ordering changes that are unexpected.

For example, .mdc-text-field--with-leading-icon {} selector is rendered after .mdc-text-field--with-leading-icon.mdc-text-field--outlined.

Since text field has lots of subtle animations not all screenshot tests would capture it.

As discussed on chat, let's try to keep the CSS ordering changes to minimal. Also it would be great if we can avoid introducing new mixins (Such as input() & outlined()) in the same PR.

Sets up feature targeting for the text field and its related styles.

Relates to material-components#4227.
@crisbeto crisbeto force-pushed the text-field-feature-targeting branch from 36985dd to f246f22 Compare January 19, 2020 11:44
@crisbeto
Copy link
Collaborator Author

@abhiomkar I managed to reduce the differences even more, but there are still some which are difficult to work around. Here's the breakdown:

  1. The order of the animation keyframes and the helper text/icon/character counter is swapped. There's no way to get around it if we want to keep everything under the core-styles mixin. I don't think this will be a problem since the styles don't affect each other.
  2. The ripple-related styles are moved out after all the other form field styles. We can't get around it since the ripple styles and the rest of the styles are in different mixins.
  3. In two places the transition property was moved to the end so that things fit neatly inside feature targets. Technically we can work around it, but the formatting would look weird and I'd expect someone to eventually refactor it.

Here are the updated before/after files.
before.css.txt
after.css.txt

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.

Thanks @crisbeto!

Internal tests are passing. Waiting for review.

cl/289157409

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.

6 participants