[TextField] Converting TextField and Resizer to functional components using React Hooks #1997

dleroux · 2019-08-20T19:49:40Z

WHY are these changes introduced?

Part of #1995

WHAT is this pull request doing?

change TextField and Resizer to use react hooks

How to 🎩

🖥 Local development instructions
🗒 General tophatting guidelines
📄 Changelog guidelines

Loading the Textfield examples in the playground and ensuring they all behave as expected should be sufficient.

Copy-paste this code in playground/Playground.tsx:

import React from 'react';
import {Page} from '../src';

export default function Playground() {
  return (
    <Page title="Playground">
      {/* Add the code you want to test in here */}
    </Page>
  );
}

🎩 checklist

Tested on mobile
Tested on multiple browsers
Tested for accessibility
Updated the component's README.md with documentation changes
Tophatted documentation changes in the style guide

dleroux · 2019-08-30T18:08:06Z

src/components/TextField/TextField.scss

@@ -29,7 +29,7 @@ $stacking-order: (
  flex-wrap: wrap;

  > .Input {
-    overflow: auto;
+    overflow: hidden;


Not sure why but I think because the effects run after the function has run, the Textarea scrollbar was flickering when multiline.

Yep the effects a ran after rendering the component. If you need some effect to happen before the dom is written you might be able to use useLayoutEffect but that bigass warning box that says useLayoutEffect and server-side render don't play nicely make me think this might be a better approach

Looks like this won't work. The height is set based on the Resizer size, there's always a delay and the TextField scrolling is janky. Needed to add useLayoutEffect in the Resizer.

CautionTapeBot · 2019-09-12T12:25:42Z

We noticed that this PR either modifies or introduces usage of the dangerouslySetInnerHTML attribute, which can cause cross-site scripting (XSS) vulnerabilities. Our team will take a look soon, but for now we recommend reviewing your code to ensure that this is what you intended to use and that there is not a safe alternative available. Docs are available here.

dleroux · 2019-09-12T12:26:52Z

We noticed that this PR either modifies or introduces usage of the dangerouslySetInnerHTML attribute, which can cause cross-site scripting (XSS) vulnerabilities. Our team will take a look soon, but for now we recommend reviewing your code to ensure that this is what you intended to use and that there is not a safe alternative available. Docs are available here.

This was not added as part of this PR.

BPScott

All looks good so far. Two comments on hook usage then I'm happy. Would appreciate a second set of eyes from somebody else though :)

Two opportunities to use new hooks - the useToggle one might be borderline and based on its usage might not be worth it, but using useUniqueId shall certainly be worth it

BPScott · 2019-09-13T00:05:27Z

src/components/TextField/TextField.tsx

+}: TextFieldProps) {
+  const intl = useI18n();
+  const [height, setHeight] = useState<number | null>(null);
+  const [focus, setFocus] = useState(focused || false);


Would it make sense use useToggle or useForcibleToggle?

I was wondering the same. Looks like it's essentially just an extra level of abstraction that is less explicit, and in the end does the same?

We could probably initialize this as useState(focused) or if we wanted to keep the type as boolean we usually use the boolean constructor useState(Boolean(focused)), no strong opinions

BPScott · 2019-09-13T00:09:20Z

src/components/TextField/TextField.tsx

+  const [focus, setFocus] = useState(focused || false);
+
+  const generatedId = useRef(getUniqueID());
+  const id = idProp || generatedId.current;


Use useUniqueId, and you can remove the getUniqueID function and generatedId.

const id = useUniqueId('TextField', idProp);

dleroux · 2019-09-16T14:34:10Z

src/components/TextField/components/Resizer/Resizer.tsx

-  };
+  }, [currentHeight, onHeightChange]);
+
+  useLayoutEffect(() => {


Need to use this so that the height gets adjusted before committing.

useLayoutEffect doesn't play well SSR, we'll probably want to rethink this -> https://gist.github.com/gaearon/e7d97cdf38a2907924ea12e4ebdf3c85

I opted for his second option since useEffect didn't work for us.

BPScott · 2019-09-16T21:44:37Z

src/components/TextField/components/Resizer/Resizer.tsx


      if (newHeight !== currentHeight) {
        onHeightChange(newHeight);
      }
    });
-  };
+  }, [currentHeight, onHeightChange]);


This is going to result in creating a new function every time the currentHeight changes. If we store currentHeight in a ref then I think we'd be able to take it out of the dependency array and thus avoid recreating the function

If we store currentHeight in a ref then I think we'd be able to take it out of the dependency array and thus avoid recreating the function

Your correct 😄 the reference to the ref will never be stale, just make sure not to deconstruct the value off the ref or that'll have to become a dependency

AndrewMusgrave · 2019-09-16T22:58:04Z

Sorry Dan I didn't get to this today, I'll 🎩 and review in the morning!

dleroux · 2019-09-17T14:10:25Z

src/components/TextField/TextField.tsx

 import {CircleCancelMinor} from '@shopify/polaris-icons';
 import {VisuallyHidden} from '../VisuallyHidden';
 import {classNames, variationName} from '../../utilities/css';
-
+import {useI18n} from '../../utilities/i18n';
+import {useMountedRef} from '../../utilities/mounted-ref';


same as @shopify/react-hooks

dleroux · 2019-09-17T14:12:36Z

src/utilities/tests/mounted-ref.test.tsx

+    expect(spy).toHaveBeenCalledWith(true);
+  });
+
+  it('returns a ref with current value as false when the component is un-mounted', async () => {


This is the same test used in @shopify/react-hooks except we are getting a warning because of the promise.

Warning: The callback passed to ReactTestUtils.act(...) function must not return anything.

Any suggestions on how we can test that unmounting returns false without it?

Got this to work, thoughts?

`
it('returns a ref with current value as false when the component is un-mounted', () => {
const spy = jest.fn((_) => {});

function MockComponent() { const mounted = useMountedRef(); useEffect(() => { return () => { // eslint-disable-next-line react-hooks/exhaustive-deps spy(mounted.current); }; }, [mounted]); return <div />; } const mockComponent = mount(<MockComponent />); mockComponent.unmount(); expect(spy).toHaveBeenCalledWith(false);

});
`

Are our type files up to date?

dleroux · 2019-09-17T14:14:16Z

Sorry Dan I didn't get to this today, I'll 🎩 and review in the morning!

No problem. I applied @BPScott suggestions and yours and brought over useMountedRef . Left you a question regarding the test.

AndrewMusgrave

Looks good! Have a concern about the useMountedRef though

AndrewMusgrave · 2019-09-17T15:37:37Z

src/components/TextField/tests/TextField.test.tsx

@@ -2,7 +2,7 @@ import React from 'react';
 import {mountWithAppProvider, findByTestID} from 'test-utilities/legacy';


Any chance you can use react-testing? We're trying not to use the legacy stuff anymore and eventually migrate our old tests away. Since you already wrote the tests though, don't feel like you have to go back and change them though.

AndrewMusgrave · 2019-09-17T15:38:34Z

src/utilities/mounted-ref.ts

+import {useRef, useEffect} from 'react';
+
+export function useMountedRef() {
+  const mounted = useRef(true);


Should this be false? 🤔

Suggested change

const mounted = useRef(true);

const mounted = useRef(false);

AndrewMusgrave · 2019-09-17T15:39:00Z

src/utilities/mounted-ref.ts

+
+  useEffect(() => {
+    return () => {
+      mounted.current = false;


Should this be true?? 🤔

Suggested change

mounted.current = false;

mounted.current = true;

Maybe I'm missing something?

This one makes sense that it's false because it's for the clean-up?

🤦‍♂ Didn't see it was in the returned function 😄

AndrewMusgrave · 2019-09-17T15:39:40Z

src/utilities/tests/mounted-ref.test.tsx

@@ -0,0 +1,72 @@
+import React from 'react';
+import {mount} from '@shopify/react-testing';


Suggested change

import {mount} from '@shopify/react-testing';

import {mount} from 'test-utilities';

AndrewMusgrave · 2019-09-17T15:40:37Z

src/utilities/tests/mounted-ref.test.tsx

+    expect(spy).toHaveBeenCalledWith(true);
+  });
+
+  it('returns a ref with current value as false when the component is un-mounted', async () => {


Are our type files up to date?

AndrewMusgrave · 2019-09-17T15:45:36Z

src/components/TextField/components/Resizer/Resizer.tsx

-    if (this.animationFrame) {
-      cancelAnimationFrame(this.animationFrame);
+  useEffect(() => {
+    return () => {


If we moved the cleanup to useLayoutEffectwe could kill this hook

AndrewMusgrave · 2019-09-17T15:49:07Z

src/components/TextField/TextField.tsx

+}: TextFieldProps) {
+  const intl = useI18n();
+  const [height, setHeight] = useState<number | null>(null);
+  const [focus, setFocus] = useState(focused || false);


We could probably initialize this as useState(focused) or if we wanted to keep the type as boolean we usually use the boolean constructor useState(Boolean(focused)), no strong opinions

AndrewMusgrave · 2019-09-17T15:50:36Z

src/components/TextField/TextField.tsx

+    if (!inputRef.current) return;
+    if (focused) {
+      inputRef.current.focus();
+    } else if (focused === false) {


Right now if focused isn't true, it'll be false, won't it? We can probably kill this else if with a return in the if statement or an else or a ternary (focused ? ...focus() : ... blur())

AndrewMusgrave · 2019-09-17T15:54:05Z

src/components/TextField/TextField.tsx

+
+  const handleButtonRelease = useCallback(() => {
+    clearTimeout(buttonPressTimer.current);
+  }, [buttonPressTimer]);


We can kill this as a dependency

Suggested change

}, [buttonPressTimer]);

}, []);

AndrewMusgrave · 2019-09-17T15:56:12Z

src/components/TextField/TextField.tsx

-
-    const resizer = multiline ? (
+  const resizer =
+    multiline && mounted.current ? (


I'm a little concerned about this, since in the hook when useEffect runs this will evaluate as false... but I could be being an airhead 😄

AndrewMusgrave

Pushed up a small change, LGTM 👍

dleroux force-pushed the Textfield-as-function branch 2 times, most recently from 979fd9c to 881e844 Compare August 21, 2019 12:30

dleroux force-pushed the Textfield-as-function branch from 881e844 to 9863ab3 Compare August 30, 2019 17:20

dleroux requested a review from amrocha August 30, 2019 17:57

dleroux commented Aug 30, 2019

View reviewed changes

dleroux force-pushed the Textfield-as-function branch from 9863ab3 to a194387 Compare September 12, 2019 12:11

dleroux changed the title ~~[WIP][TextField] Converting TextField to a functional component~~ [TextField] Converting TextField and Resizer to functional components using React Hooks Sep 12, 2019

shopify-admins requested a review from a team September 12, 2019 12:25

dleroux requested a review from BPScott September 12, 2019 12:25

BPScott reviewed Sep 13, 2019

View reviewed changes

dleroux force-pushed the Textfield-as-function branch 2 times, most recently from 9783dc9 to c1e5d9a Compare September 13, 2019 13:36

dleroux requested a review from AndrewMusgrave September 13, 2019 13:38

BPScott mentioned this pull request Sep 13, 2019

Post v4 cleanup work #1995

Closed

dleroux force-pushed the Textfield-as-function branch from c1e5d9a to f577dbb Compare September 16, 2019 14:31

dleroux commented Sep 16, 2019

View reviewed changes

dleroux requested a review from BPScott September 16, 2019 14:34

BPScott reviewed Sep 16, 2019

View reviewed changes

dleroux force-pushed the Textfield-as-function branch 3 times, most recently from a55fee1 to 1d7fe03 Compare September 17, 2019 14:08

dleroux commented Sep 17, 2019

View reviewed changes

dleroux requested a review from BPScott September 17, 2019 14:30

AndrewMusgrave reviewed Sep 17, 2019

View reviewed changes

dleroux force-pushed the Textfield-as-function branch from 1d7fe03 to 0642c4d Compare September 17, 2019 16:52

converting Textfield to function

ed0ffa7

dleroux force-pushed the Textfield-as-function branch from 0642c4d to ed0ffa7 Compare September 17, 2019 19:32

Use react-testing and update effect

374a05b

AndrewMusgrave approved these changes Sep 17, 2019

View reviewed changes

Fix extra import

10b7285

dleroux merged commit 0b9938e into master Sep 17, 2019

dleroux deleted the Textfield-as-function branch September 17, 2019 21:33

dleroux temporarily deployed to production September 20, 2019 17:44 Inactive

dleroux temporarily deployed to production September 23, 2019 15:28 Inactive

dleroux temporarily deployed to production September 23, 2019 15:38 Inactive

dleroux temporarily deployed to production September 23, 2019 16:59 Inactive

dleroux temporarily deployed to production September 23, 2019 17:05 Inactive

		@@ -2,7 +2,7 @@ import React from 'react';
		import {mountWithAppProvider, findByTestID} from 'test-utilities/legacy';

		@@ -0,0 +1,72 @@
		import React from 'react';
		import {mount} from '@shopify/react-testing';

	import {mount} from '@shopify/react-testing';
	import {mount} from 'test-utilities';

[TextField] Converting TextField and Resizer to functional components using React Hooks #1997

[TextField] Converting TextField and Resizer to functional components using React Hooks #1997

Conversation

dleroux commented Aug 20, 2019 • edited Loading

WHY are these changes introduced?

WHAT is this pull request doing?

How to 🎩

🎩 checklist

Choose a reason for hiding this comment

BPScott Sep 12, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CautionTapeBot commented Sep 12, 2019

dleroux commented Sep 12, 2019

BPScott 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

AndrewMusgrave commented Sep 16, 2019

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

dleroux commented Sep 17, 2019 • edited Loading

AndrewMusgrave left a comment

Choose a reason for hiding this comment

AndrewMusgrave Sep 17, 2019 • 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

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

AndrewMusgrave left a comment

Choose a reason for hiding this comment

dleroux commented Aug 20, 2019 •

edited

Loading

BPScott Sep 12, 2019 •

edited

Loading

dleroux commented Sep 17, 2019 •

edited

Loading

AndrewMusgrave Sep 17, 2019 •

edited

Loading