Text field boxes + Ripple Improvements #801

traviskaufman · 2017-06-08T14:49:17Z

This PR constitutes a number of commits which resolve #673 and should be rebased and merged.

fix(ripple): Remove fg deactivation class when animation finishes
feat(ripple): Add layout() method to component
feat(ripple): Reduce the fade out time for foreground ripple effect
feat(textfield): Implement text field boxes

codecov-io · 2017-06-08T14:59:13Z

Codecov Report

Merging #801 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #801      +/-   ##
==========================================
+ Coverage   99.93%   99.93%   +<.01%     
==========================================
  Files          68       68              
  Lines        3134     3144      +10     
  Branches      384      387       +3     
==========================================
+ Hits         3132     3142      +10     
  Misses          2        2

Impacted Files	Coverage Δ
packages/mdc-textfield/constants.js	`100% <ø> (ø)`	⬆️
packages/mdc-ripple/constants.js	`100% <ø> (ø)`	⬆️
packages/mdc-ripple/foundation.js	`100% <100%> (ø)`	⬆️
packages/mdc-textfield/index.js	`100% <100%> (ø)`	⬆️
packages/mdc-ripple/index.js	`100% <100%> (ø)`	⬆️

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 307305c...4c45efb. Read the comment docs.

yeelan0319

Here is a weird bug on my end.

If you select dark mode, right click on the box, left click outside the box to turn off the menu, left click again outside the box. You will see:

Not sure what causes this yet, @traviskaufman maybe you have a quick answer?

yeelan0319 · 2017-06-09T18:19:54Z

packages/mdc-textfield/mdc-textfield.scss

+    position: absolute;
+    top: 0;
+    left: 0;
+    // TODO ripple positioning logic?


Is this associate with other issues? Feel better to leave a reference here

Old TODO :p removing

yeelan0319 · 2017-06-09T18:28:11Z

test/unit/mdc-textfield/mdc-textfield.test.js

+  assert.isNull(component.ripple);
+});
+
+test('#constructor initializes a default ripple when no ripple factory given when given a ' +


How about "constructor initializes a default ripple when ripple factory is not given in mdc-textfield--box element?"

That to me sounds like there's some "ripple factory" inside of an mdc-textfield--box element. How about: "#constructor when given a mdc-textfield--box element, initializes a default ripple when no ripple factory given"

yeelan0319 · 2017-06-09T18:36:40Z

test/unit/mdc-textfield/mdc-textfield.test.js

 function setupTest() {
  const root = getFixture();
  const component = new MDCTextfield(root);
  return {root, component};
 }

-test('#adapter.initialSyncWithDom', () => {
+test('#initialSyncWithDom', () => {


How about "#initialSyncWithDom set disable to false"?

amsheehan

One question and a few small issues, otherwise looks good!

amsheehan · 2017-06-12T14:46:14Z

packages/mdc-textfield/mdc-textfield.scss

+    bottom: 0;
+    left: 0;
+    width: 100%;
+    height: 1px;


Should this be higher? Spec says even when idle and empty, it should be 2dp, and the visual examples make the bottom line look taller.

Yeah so this is a change that hasn't yet made it into spec. When we implement #675 it'll elevate to 2dp on hover, and stay at 2dp on focus.

amsheehan · 2017-06-12T14:52:45Z

demos/textfield.html

+              tfEl.classList[evt.target.checked ? 'add' : 'remove']('mdc-textfield--dense');
+              tf.ripple.layout();
+            });
+          }, 80);


Doesn't need to be 80. Can just be 0 so it's at the end of the execution queue. I feel like potentially the issue is that by us using an arbitrary value, users just cutting and pasting code may think they need to do the same thing. To me, 0 seems like less of a directive.

Yeah agreed that 0 is better; I didn't realize we could use 0. Hopefully sometime soon we'll have a more robust solution for loading these demo styles.

amsheehan · 2017-06-12T14:54:47Z

packages/mdc-ripple/README.md

@@ -181,6 +181,10 @@ such as a `mousedown` or a `pointerdown` event). It expands from the center.
 Triggers a deactivation of the ripple (the second stage, which happens when the ripple surface is engaged via
 interaction, such as a `mouseup` or a `pointerup` event). It expands from the center.

+#### MDCRipple.layout()


amsheehan · 2017-06-12T15:41:52Z

packages/mdc-textfield/mdc-textfield.scss

+
+  &.mdc-ripple-upgraded {
+    @include mdc-ripple-base;
+    // TODO: Add ::before as ripple bg once #194 lands


It looks like the background fade out on the hover ripple doesn't exist, making it seem like it disappears very abruptly on interaction end events (moving the pointer out of the target). Do you have any sense of whether or not this component will receive ink wash on hover as a result of #194 ?

If it does not I think it will solve this problem as the ripple on click interactions fades out as expected.

@amsheehan yeah it will. There is a fade out, but it is very quick (<100ms). You can see it in devtools if you slow down the animations using the "Animations" tab. I worked with motion design on it and they signed off on it, so I'm assuming it's correct?

Curious. I had initially slowed it down but still couldn't see it animating out. I'm thinking the darker background of the text box background is the same color as the ink wash, which makes it appear as though there is no fade out. All the user can see is the foreground ripple (which doesn't animate out afaik)

Either way, if motion signed off, that's good enough for me. 👍

amsheehan · 2017-06-12T15:57:36Z

Can I make a request that we make the docs changes to full width text fields part of this PR? Essentially we need to note that full width text fields do not support labels. Eh?

- Remove foreground deactivation class after fg-opacity-out animation finishes such that additional repaints for the element don't retrigger animations.

Change made after additional review from the team's motion designers.

traviskaufman · 2017-06-13T20:27:42Z

@amsheehan I can add the note about full width textfields as part of this PR for sure.

traviskaufman · 2017-06-13T20:48:04Z

@yeelan0319 @amsheehan PTAL!

@yeelan0319 That bug looks like it's for the ripple. I will file an issue for it.

yeelan0319 · 2017-06-14T20:19:00Z

@traviskaufman SGTM. LGTMed this PR.

Resolves #673

…elds

traviskaufman added the rebase-and-merge label Jun 8, 2017

traviskaufman requested review from amsheehan and yeelan0319 June 8, 2017 14:49

traviskaufman added this to the Textfield Improvements milestone Jun 8, 2017

yeelan0319 suggested changes Jun 9, 2017

View reviewed changes

amsheehan suggested changes Jun 12, 2017

View reviewed changes

traviskaufman added 3 commits June 12, 2017 17:27

fix(ripple): Remove fg deactivation class when animation finishes

238d385

- Remove foreground deactivation class after fg-opacity-out animation finishes such that additional repaints for the element don't retrigger animations.

feat(ripple): Add layout() method to component

ed1d571

feat(ripple): Reduce the fade out time for foreground ripple effect

96df7b3

Change made after additional review from the team's motion designers.

traviskaufman force-pushed the feat/text-field-box branch from 89be862 to b773120 Compare June 13, 2017 20:41

amsheehan approved these changes Jun 14, 2017

View reviewed changes

yeelan0319 approved these changes Jun 14, 2017

View reviewed changes

traviskaufman added 2 commits June 14, 2017 17:24

feat(textfield): Implement text field boxes

abd5fae

Resolves #673

docs(textfield): Add note about no label support in full-width textfi…

4c45efb

…elds

traviskaufman force-pushed the feat/text-field-box branch from b773120 to 4c45efb Compare June 14, 2017 21:25

traviskaufman merged commit 184897b into master Jun 15, 2017

traviskaufman deleted the feat/text-field-box branch June 15, 2017 16:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Text field boxes + Ripple Improvements #801

Text field boxes + Ripple Improvements #801

traviskaufman commented Jun 8, 2017

codecov-io commented Jun 8, 2017 •

edited

Loading

yeelan0319 left a comment •

edited

Loading

yeelan0319 Jun 9, 2017

traviskaufman Jun 12, 2017

yeelan0319 Jun 9, 2017

traviskaufman Jun 12, 2017

yeelan0319 Jun 9, 2017

amsheehan left a comment

amsheehan Jun 12, 2017

traviskaufman Jun 13, 2017

amsheehan Jun 14, 2017

amsheehan Jun 12, 2017

traviskaufman Jun 13, 2017

amsheehan Jun 12, 2017

amsheehan Jun 12, 2017

traviskaufman Jun 13, 2017

amsheehan Jun 14, 2017

amsheehan commented Jun 12, 2017

traviskaufman commented Jun 13, 2017

traviskaufman commented Jun 13, 2017

yeelan0319 commented Jun 14, 2017

Text field boxes + Ripple Improvements #801

Text field boxes + Ripple Improvements #801

Conversation

traviskaufman commented Jun 8, 2017

codecov-io commented Jun 8, 2017 • edited Loading

Codecov Report

yeelan0319 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amsheehan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amsheehan commented Jun 12, 2017

traviskaufman commented Jun 13, 2017

traviskaufman commented Jun 13, 2017

yeelan0319 commented Jun 14, 2017

codecov-io commented Jun 8, 2017 •

edited

Loading

yeelan0319 left a comment •

edited

Loading