Update textfield to Polaris v3.10.0 #310

tomnez · 2019-04-02T18:28:59Z

Adds a new showCharacterCount attribute as well as methods for holding down the buttons in the number spinners to slowly increment/decrement the current number value.

Character count with max length:

Character count with no max length:

Press and hold spinner:

tomnez · 2019-04-02T18:34:44Z

addon/components/polaris-text-field.js

-    if (input && focused) {
-      // TODO this _seems_ to work in Polaris without the row below, but not for us (it does not apply focus class)
-      // look what's here....though this seems to do the job for now
-      this.set('focus', true);


This was removed in favor of the focus computed property below now that the react implementation has updated their code to check the previous and current value of focused in order to determine whether or not to focus or blur the field.

We can't really do the same thing and call blur on didUpdateAttrs because there's a chance other attributes have been updated while focused is set to false, and we wouldn't want to accidentally blur the field as the user is typing just because the initial value of focused was false.

We can't really do the same thing and call blur on didUpdateAttrs because there's a chance other attributes have been updated while focused is set to false, and we wouldn't want to accidentally blur the field as the user is typing just because the initial value of focused was false.

Not sure I follow this 😬 Can't we track wasFocused internally and call blur/focus as appropriate from didUpdateAttrs? If focused changes, then we should still be obeying it, and if you're worried about "accidentally blurring the field as the user is typing", that's on the host app to pass sensible property values 🤷‍♂️

I guess we can make some bootleg version of wasFocused. The original buggyness I was seeing wasn't a host app being irresponsible, it was that you couldn't type without the field blur-ing unless you explicitly passed focused=true, which shouldn't happen. I'll see about making our own wasFocused.

Yeah, I don't see why this shouldn't work with wasFocused (we're doing similar things in some other components already)

andrewpye

Any idea why the native spinner is showing here?

It doesn't seem to appear on the React implementation... 😬

Otherwise this looks pretty sick!

andrewpye · 2019-04-03T10:05:25Z

addon/components/polaris-text-field.js

-    let { input, focused } = this.getProperties('input', 'focused');
+  normalizedValue: computed('value', function() {
+    let value = this.get('value');
+    return value !== null ? value : '';


Might be worth explicitly matching the React implementation and using != here so that this catches undefined values etc.

andrewpye · 2019-04-03T10:05:52Z

addon/components/polaris-text-field.js

-      // look what's here....though this seems to do the job for now
-      this.set('focus', true);
-      input.focus();
+  characterCount: computed('normalizedValue.length', function() {


This should maybe be readOnly

andrewpye · 2019-04-03T10:06:01Z

addon/components/polaris-text-field.js

+  normalizedValue: computed('value', function() {
+    let value = this.get('value');
+    return value !== null ? value : '';
+  }),


Can this be readOnly?

andrewpye · 2019-04-03T10:07:27Z

addon/components/polaris-text-field.js

+    return maxLength
+      ? `${characterCount} characters of ${maxLength} used`
+      : `${characterCount} characters`;
+  }),


readOnly?

andrewpye · 2019-04-03T10:07:33Z

addon/components/polaris-text-field.js

-  },
+
+    return classNames.join(' ');
+  }),


readOnly?

andrewpye · 2019-04-03T10:13:42Z

addon/components/polaris-text-field.js

-    if (input && focused) {
-      // TODO this _seems_ to work in Polaris without the row below, but not for us (it does not apply focus class)
-      // look what's here....though this seems to do the job for now
-      this.set('focus', true);


We can't really do the same thing and call blur on didUpdateAttrs because there's a chance other attributes have been updated while focused is set to false, and we wouldn't want to accidentally blur the field as the user is typing just because the initial value of focused was false.

Not sure I follow this 😬 Can't we track wasFocused internally and call blur/focus as appropriate from didUpdateAttrs? If focused changes, then we should still be obeying it, and if you're worried about "accidentally blurring the field as the user is typing", that's on the host app to pass sensible property values 🤷‍♂️

andrewpye · 2019-04-03T10:15:40Z

addon/components/polaris-text-field.js

+  }),
+
+  focus: computed('focused', function() {
+    return this.get('focused') || false;


lol, this is weird logic but I appreciate that it's in the React implementation 🤷‍♂️ Anyhoo, how 'bout making this readOnly too?

andrewpye · 2019-04-03T10:20:47Z

addon/components/polaris-text-field.js

+
+      this.set(
+        'buttonPressTimer',
+        window.setTimeout(onChangeInterval, interval)


I wonder if we're better off using runloop-compatible timer functions here 🤔 This is probably fine in React-land but it's making me nervous in Ember-world 😅

vladucu

Some minor comments & I think we missed updating ariaDescribedBy CP....but almost there

vladucu · 2019-04-04T18:06:39Z

addon/components/polaris-text-field.js

-    let { input, focused } = this.getProperties('input', 'focused');
+  normalizedValue: computed('value', function() {
+    let value = this.get('value');
+    return value !== null ? value : '';


vladucu · 2019-04-04T18:11:42Z

addon/templates/components/polaris-text-field.hbs

@@ -45,7 +45,7 @@
          minlength=minLength
          maxlength=maxLength
          spellcheck=spellCheck
-          value=value
+          value=normalizedValue


we should also pass step here?

vladucu · 2019-04-04T18:20:48Z

addon/components/polaris-text-field/spinner.js

+
+  onMouseDown() {},
+
+  onMouseUp(/* onChange */) {},


nit: onMouseDown gets invoked with onChange, not onMouseUp 😜

vladucu · 2019-04-05T16:04:41Z

Any idea why the native spinner is showing here?

It doesn't seem to appear on the React implementation... 😬

@tomnez any luck with this ^ ?

tomnez · 2019-04-05T16:24:46Z

@vladucu ah missed that too, I'll look into it.

vladucu · 2019-04-05T16:27:01Z

thx!

andrewpye

Giving this the green light to merge into the epic branch, where it'll be easier to check out the number spinner issue to see whether it's a dummy app thing or something else 👍

tomnez · 2019-04-08T14:47:23Z

I'm having wacky node issues with linking the host apps to this branch, we suspect this is just a dummy app issue since the React markup also displays the same problem when copied into the dummy app so we're going to merge and make a note to double check this on a real app.

tomnez self-assigned this Apr 2, 2019

tomnez requested review from vladucu and andrewpye April 2, 2019 18:29

tomnez commented Apr 2, 2019

View reviewed changes

tomnez added 4 commits April 2, 2019 14:17

Updates to polaris-text-field

f0881f4

Update text-field and subcomponents to 3.10.0

69aaeab

Fix tests, add new test for characterCount

f7afd07

Lint the things

814eb3c

tomnez force-pushed the update/textfield-v3.10.0 branch from fc92356 to 814eb3c Compare April 2, 2019 19:18

andrewpye reviewed Apr 3, 2019

View reviewed changes

tomnez and others added 3 commits April 3, 2019 10:16

Create wasFocused property

c1e3260

readOnly all the things

7d22e0d

Use ember-lifeline for button press/release handling

e1074e7

tomnez requested a review from andrewpye April 3, 2019 17:27

vladucu reviewed Apr 4, 2019

View reviewed changes

Missed edits, fix arg comment

aec271b

tomnez requested a review from vladucu April 5, 2019 15:27

andrewpye approved these changes Apr 8, 2019

View reviewed changes

tomnez merged commit 27b767f into update/polaris-v3.10.0 Apr 8, 2019

delete-merged-branch bot deleted the update/textfield-v3.10.0 branch April 8, 2019 14:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update textfield to Polaris v3.10.0 #310

Update textfield to Polaris v3.10.0 #310

tomnez commented Apr 2, 2019

tomnez Apr 2, 2019

andrewpye Apr 3, 2019

tomnez Apr 3, 2019

andrewpye Apr 3, 2019

andrewpye left a comment

andrewpye Apr 3, 2019

vladucu Apr 4, 2019

andrewpye Apr 3, 2019

andrewpye Apr 3, 2019

andrewpye Apr 3, 2019

andrewpye Apr 3, 2019

andrewpye Apr 3, 2019

andrewpye Apr 3, 2019

andrewpye Apr 3, 2019

vladucu Apr 3, 2019

vladucu left a comment

vladucu Apr 4, 2019

vladucu Apr 4, 2019

vladucu Apr 4, 2019

vladucu commented Apr 5, 2019

tomnez commented Apr 5, 2019

vladucu commented Apr 5, 2019

andrewpye left a comment

tomnez commented Apr 8, 2019

Update textfield to Polaris v3.10.0 #310

Update textfield to Polaris v3.10.0 #310

Conversation

tomnez commented Apr 2, 2019

Character count with max length:

Character count with no max length:

Press and hold spinner:

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

andrewpye 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

Choose a reason for hiding this comment

vladucu 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

vladucu commented Apr 5, 2019

tomnez commented Apr 5, 2019

vladucu commented Apr 5, 2019

andrewpye left a comment

Choose a reason for hiding this comment

tomnez commented Apr 8, 2019