fix(textfield): Update label position on value changed via JS #1197

anton-kachurin · 2017-08-28T03:11:10Z

Fixes #822

codecov-io · 2017-08-28T03:22:42Z

Codecov Report

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

@@            Coverage Diff             @@
##           master    #1197      +/-   ##
==========================================
+ Coverage   99.82%   99.82%   +<.01%     
==========================================
  Files          69       69              
  Lines        3388     3433      +45     
  Branches      424      431       +7     
==========================================
+ Hits         3382     3427      +45     
  Misses          6        6

Impacted Files	Coverage Δ
packages/mdc-textfield/constants.js	`100% <ø> (ø)`	⬆️
packages/mdc-textfield/foundation.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 874a17e...04ac0e8. Read the comment docs.

lynnmercier · 2017-10-12T17:49:41Z

Sorry @anton-kachurin, we did a lot of text field updates in a big change, and it has caused merge conflicts in this PR. Can you resolve them? Or it might be better to just create a whole new PR.

touficbatache · 2017-10-14T19:50:12Z

@anton-kachurin What's up with this? Would love to see it implemented in MDC

anton-kachurin · 2017-10-14T20:06:14Z

@touficbatache I've been waiting for a review for longer than a month and the activity log clearly shows that this PR isn't abandoned. At this very moment I'm working on resolving merge conflicts.

I don't consider your comment appropriate. This is an open source project, and if you personally would love to see something in it, go ahead and implement it. You always have an option to fork from my fork and take the branch to the point where it can be merged to master. Don't press on volunteers, we're doing it in a spare time.

As a part of the community, please be respectful to others.

touficbatache · 2017-10-14T20:09:45Z

I'm sorry, but I'm not being disrespectful. I'm just telling that your work is great by saying that I'd love seeing it implemented, I didn't say that to put pressure on anybody. Have a great day.

gjdev · 2017-10-16T19:29:52Z

AFAIK this pull request only updates the label-float style after a programmatic change of the input value. However, a change of the input value may affect other styles when the new value introduces or fixes a previous input value error. Hence value changes may also affect the error styles, helptext, and errortext.

anton-kachurin · 2017-10-16T23:44:11Z

packages/mdc-textfield/foundation.js

+      if (!this.useCustomValidityChecking_) {
+        this.adapter_.removeClassFromLabel(LABEL_SHAKE);
+        this.showHelptext_();
+        this.changeValidity_(input.checkValidity());


@gjdev Validation is handled in this PR

Sorry I missed that somehow.

lynnmercier · 2017-10-19T23:27:28Z

Hey @anton-kachurin, I wrote a proposal for a different way to solve #822 (in the #822 thread). Can you check it out and comment on that thread. I'd like to hash out some code architecture details before I can review any code changes to solve #822.

lynnmercier

Left some initial questions. I'll try and review the rest of it soon.

lynnmercier · 2017-11-09T17:19:10Z

packages/mdc-textfield/foundation.js

+
+    if (this.isFocused_ || input.value) {
+      if (!this.labelFloatIsActive_) {
+        this.labelFloatIsActive_ = true;


instead of you using this.labelFloatIsActive as a internal state variable to keep track of "if the LABEL_FLOAT_ABOVE class has been added". Why not add a method to the adapter to check adapter_.labelHasClass(LABEL_FLOAT_ABOVE)?

lynnmercier · 2017-11-09T17:20:31Z

packages/mdc-textfield/foundation.js

+        this.adapter_.addClassToLabel(LABEL_FLOAT_ABOVE);
+      }
+    } else {
+      if (this.labelFloatIsActive_) {


you actually don't even need this check. It doesnt matter what the internal state of this.labelFloatIsActive is....if neither this.isFocused_ or input.value is true, then you should remove LABEL_FLOAT_ABOVE from the label element.

lynnmercier · 2017-11-09T17:21:44Z

packages/mdc-textfield/foundation.js

    this.isFocused_ = false;
    this.adapter_.removeClass(FOCUSED);
-    this.adapter_.removeClassFromLabel(LABEL_SHAKE);

    if (!input.value && !this.isBadInput_()) {


should this.isBadInput be checked within the logic of updateLabelFloat?

anton-kachurin · 2017-11-10T17:26:09Z

@lynnjepsen Regarding your questions above:

Why these conditions were added:

      if (!this.labelFloatIsActive_) {
        this.labelFloatIsActive_ = true;
        this.adapter_.addClassToLabel(LABEL_FLOAT_ABOVE);
      }

      if (this.labelFloatIsActive_) {
        this.labelFloatIsActive_ = false;
        this.adapter_.removeClassFromLabel(LABEL_FLOAT_ABOVE);
      }

Before, the LABEL_FLOAT_ABOVE class was only added/removed on focus/blur events. These event always come in pairs, so we could assume that if onfocus is processing, the label is not floating at the moment. And vice versa.

Now we extend the foundation with the setValue method, and this method may be called at any arbitrary moment. That means the following situations should be assumed:

The user focuses the input, then blurs it without entering any value, then setValue('') is called. In pseudocode:

addClass(FLOAT);
removeClass(FLOAT);
removeClass(FLOAT);

The user focuses the input, then setValue('not empty') is called:

addClass(FLOAT);
addClass(FLOAT);

The flag this.labelFloatIsActive_ is here for filtering those duplicate calls for adding/removing the class.

The one main reason why I think it's better to double check before calling adapter_.(add/remove)ClassFromLabel is the custom adapters. While the default adapter implementation will discard duplicate addClass/removeClass calls with no side effects, we can't be sure that the custom ones will do the same. That's why in both cases the condition preventing duplicate calls was added. Please let me know if it should stay or not.

Why the boolean flag (this.labelFloatIsActive_) to maintain the internal state and not adapter_.labelHasClass(LABEL_FLOAT_ABOVE):

My argument was that the LABEL_FLOAT_ABOVE is a completely internal state but then I realized that it is not:
https://github.com/material-components/material-components-web/tree/master/packages/mdc-textfield#pre-filled-text-fields

With this in mind, we have to add adapter_.labelHasClass method.

isBadInput:
https://codepen.io/anon/pen/xPqPKO?editors=1010

this.updateLabelFloat is called in two situations:

The user manipulates with the input (onfocus/onblur)
The value is set programmatically (setValue)

The pen above shows that setting the input.value = 'invalid value' programmatically would never cause input.validity.badInput to be false. That's why we only need to check isBadInput on blur

lynnmercier

Hey Anton, This looks pretty good in general, but I want you to remove the installPropertyChangeHooks from this PR. You can create another PR after this with the installPropertyChangeHooks, but doing both in both PR is a little too hard to review.

Also were (still) doing a fair amount of changes in mdc-textfield directory, so I'm trying to reduce merge conflicts while still getting some of your changes in.

lynnmercier · 2017-11-14T21:59:39Z

packages/mdc-textfield/foundation.js

+        this.labelFloatIsActive_ = true;
+        this.adapter_.addClassToLabel(LABEL_FLOAT_ABOVE);
+      }
+    } else {


combine this else, and the if statement on the next line.

lynnmercier · 2017-11-14T22:03:29Z

packages/mdc-textfield/foundation.js

@@ -110,6 +112,7 @@ class MDCTextfieldFoundation extends MDCFoundation {
      this.adapter_.registerTextFieldInteractionHandler(evtType, this.textFieldInteractionHandler_);
    });
    this.adapter_.registerTransitionEndHandler(this.transitionEndHandler_);
+    this.installPropertyChangeHooks_();


Add these installPropertyChangeHooks in the next PR. Its a little too hard to review otherwise. And I think that means you don't need the this.setValueIsProcessing_ field.

lynnmercier · 2017-11-14T22:06:27Z

packages/mdc-textfield/foundation.js

      this.adapter_.addClass(INVALID);
+
+      if (this.isFocused_) {


is there a test for this case? A test for when the text fields value is changed programmatically, but the user is not focused on that text field. You should verify that this.adapter_.addClassToLabel(LABEL_SHAKE) is not called.

Returns whether or not the label element contains the given class.

anton-kachurin · 2017-11-18T22:18:09Z

Unfortunately, multiple merge conflict do not allow me to keep working on this PR. Besides, the main reason for me to work on it was to implement a specific feature. Instead, I was asked to refactor the code and not to include the feature, and now all this work is gone.

anton-kachurin · 2017-11-19T05:23:10Z

The work is continued in #1615

JordyvA · 2017-12-18T12:07:26Z

Is this issue also related to the new labels of the mdc-select (MDC 0.27.0)? When I set the selectedIndex over there on init, the label is written over the selected value.

This was referenced Aug 28, 2017

Floating label state anton-kachurin/mdc-knockout#3

Open

Reset MDC TextField? #924

Open

touficbatache mentioned this pull request Sep 9, 2017

Update Textfield after changing the value #1236

Closed

anton-kachurin force-pushed the fix/textfield-label-float branch from ca6d9eb to 5db7c8e Compare October 14, 2017 21:00

anton-kachurin commented Oct 16, 2017

View reviewed changes

lynnmercier mentioned this pull request Oct 19, 2017

Update textfield label position when value is changed from JavaScript #822

Closed

lynnmercier self-assigned this Nov 1, 2017

anton-kachurin force-pushed the fix/textfield-label-float branch from b40a84e to 4bbe551 Compare November 6, 2017 01:34

lynnmercier suggested changes Nov 9, 2017

View reviewed changes

lynnmercier suggested changes Nov 14, 2017

View reviewed changes

anton-kachurin added 9 commits November 14, 2017 21:27

fix(textfield): Update label position on value changed via JS

f88cc29

fix(textfield): Typecase for closure

13c877b

fix(textfield): Rewrite a test

4bef2c7

refactor(textfield): Unwrap submethod calls

91014d2

fix(textfield): Fix lint errors

4ceae27

test(textfield): Improve test coverage

0fbb93d

refactor(textfield): Refactor foundation class

08c11b3

test(textfield): Improve test coverage

cd71a42

fix(textfield): Add function docs

4ca4db6

anton-kachurin force-pushed the fix/textfield-label-float branch from 698b62a to 4ca4db6 Compare November 15, 2017 02:29

fix(text-field): Fix typos

6863506

anton-kachurin force-pushed the fix/textfield-label-float branch from a9f0e76 to 6863506 Compare November 15, 2017 03:02

feat(text-field): Add adapter.labelHasClass method

04ac0e8

Returns whether or not the label element contains the given class.

anton-kachurin closed this Nov 18, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(textfield): Update label position on value changed via JS #1197

fix(textfield): Update label position on value changed via JS #1197

anton-kachurin commented Aug 28, 2017

codecov-io commented Aug 28, 2017 •

edited

Loading

lynnmercier commented Oct 12, 2017

touficbatache commented Oct 14, 2017

anton-kachurin commented Oct 14, 2017 •

edited

Loading

touficbatache commented Oct 14, 2017

gjdev commented Oct 16, 2017

anton-kachurin Oct 16, 2017

gjdev Oct 17, 2017

lynnmercier commented Oct 19, 2017

lynnmercier left a comment

lynnmercier Nov 9, 2017

lynnmercier Nov 9, 2017

lynnmercier Nov 9, 2017

anton-kachurin commented Nov 10, 2017 •

edited

Loading

lynnmercier left a comment

lynnmercier Nov 14, 2017

lynnmercier Nov 14, 2017

lynnmercier Nov 14, 2017

anton-kachurin commented Nov 18, 2017

anton-kachurin commented Nov 19, 2017

JordyvA commented Dec 18, 2017

fix(textfield): Update label position on value changed via JS #1197

fix(textfield): Update label position on value changed via JS #1197

Conversation

anton-kachurin commented Aug 28, 2017

codecov-io commented Aug 28, 2017 • edited Loading

Codecov Report

lynnmercier commented Oct 12, 2017

touficbatache commented Oct 14, 2017

anton-kachurin commented Oct 14, 2017 • edited Loading

touficbatache commented Oct 14, 2017

gjdev commented Oct 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lynnmercier commented Oct 19, 2017

lynnmercier 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

anton-kachurin commented Nov 10, 2017 • edited Loading

lynnmercier 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

anton-kachurin commented Nov 18, 2017

anton-kachurin commented Nov 19, 2017

JordyvA commented Dec 18, 2017

codecov-io commented Aug 28, 2017 •

edited

Loading

anton-kachurin commented Oct 14, 2017 •

edited

Loading

anton-kachurin commented Nov 10, 2017 •

edited

Loading