[FilterValueSelector] Convert to functional component & add isMountedRef #2167

AndrewMusgrave · 2019-09-18T23:08:35Z

WHY are these changes introduced?

Part of #1995

WHAT is this pull request doing?

Converting FilterValueSelector to a functional component
Add useIsMountedRef hook

How to 🎩

Playground with filters below

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

import React from 'react';
import {ResourceList, TextStyle, Avatar, FilterType, Card} from '../src';

export class Playground extends React.Component {
  state = {
    searchValue: '',
    appliedFilters: [
      {
        key: 'accountStatusFilter',
        value: 'Account enabled',
      },
    ],
  };

  handleSearchChange = (searchValue) => {
    this.setState({searchValue});
  };

  handleFiltersChange = (appliedFilters) => {
    this.setState({appliedFilters});
  };

  renderItem = (item) => {
    const {id, url, name, location} = item;
    const media = <Avatar customer size="medium" name={name} />;

    return (
      <ResourceList.Item id={id} url={url} media={media}>
        <h3>
          <TextStyle variation="strong">{name}</TextStyle>
        </h3>
        <div>{location}</div>
      </ResourceList.Item>
    );
  };

  render() {
    const resourceName = {
      singular: 'customer',
      plural: 'customers',
    };

    const items = [
      {
        id: 341,
        url: 'customers/341',
        name: 'Mae Jemison',
        location: 'Decatur, USA',
      },
      {
        id: 256,
        url: 'customers/256',
        name: 'Ellen Ochoa',
        location: 'Los Angeles, USA',
      },
    ];

    const filters = [
      {
        key: 'orderCountFilter',
        label: 'Number of orders',
        operatorText: 'is greater than',
        type: FilterType.TextField,
      },
      {
        key: 'accountStatusFilter',
        label: 'Account status',
        operatorText: 'is',
        type: FilterType.Select,
        options: ['Enabled', 'Invited', 'Not invited', 'Declined'],
      },
    ];

    const filterControl = (
      <ResourceList.FilterControl
        filters={filters}
        appliedFilters={this.state.appliedFilters}
        onFiltersChange={this.handleFiltersChange}
        searchValue={this.state.searchValue}
        onSearchChange={this.handleSearchChange}
        additionalAction={{
          content: 'Save',
          onAction: () => console.log('New filter saved'),
        }}
      />
    );

    return (
      <Card>
        <ResourceList
          resourceName={resourceName}
          items={items}
          renderItem={this.renderItem}
          filterControl={filterControl}
        />
      </Card>
    );
  }
}

🎩 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
For visual design changes, pinged one of @ HYPD, @ mirualves, @ sarahill, or @ ry5n to update the Polaris UI kit

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

tmlayton · 2019-09-25T19:55:55Z

Sorry for the late notice on this since a lot of the work has been done, but the FilterControl is being removed in v5 in lieu of the Filters component.

tmlayton · 2019-09-25T19:56:46Z

Specifically it was removed in #2047

BPScott

Not ran the code but mostly seems reasonable.

I'm a bit concerned by the initial value of the hook - it seems like it would return false during the first render which seems wrong. Try running things not in strict mode (which causes a double render and see what happens?

BPScott · 2019-09-25T23:07:52Z

src/utilities/use-is-mounted-ref.ts

+ * @returns MutableRefObject<boolean> The mounted status
+ */
+export function useIsMountedRef(): MutableRefObject<boolean> {
+  const isMounted = useRef(false);


Should this be true so the hook returns true on a component's first render?

The component isn't mounted during the first render, however, it'll be mounted when the effects run? afaik

From http://projects.wojtekmaj.pl/react-lifecycle-methods-diagram/ and https://github.com/donavon/hook-flow it looks like you're right - I guess those "React updates DOM " phases are when something goes from not being mounted to is mounted so in that render before first mount a component is not mounted. I was thrown because the equivalent hook in quilt sets this to true, but it seems like that's a bug on their side: https://github.com/Shopify/quilt/blob/master/packages/react-hooks/src/hooks/mounted-ref.ts

I had mentioned this to @michenly and it sounds like this is the intention. From slack: I think you should think of this line more like a constructor of the class and the value will remain true till componentWillUnmount... it is not equal to componentDidMount... I wonder if it should be renamed to useUnMountedRef,

Typically where we are using useMountedRef from quilt are for checking if something had become un-mounted, so even thought the implementation is not exactly accurate it had been working so far.

I would like to fix it as a bug, but wonder if we would run into any situation where going from false -> true - > false will introduce a flash in UI. So this might need to be a breaking change with a more clear new name to avoid confusion.

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

AndrewMusgrave · 2019-09-30T20:22:46Z

@tmlayton No worries 😄 It was a fairly fast refactor and we still might as well 🚢 this incase we can remove withAppProvider before v5

AndrewMusgrave · 2019-09-30T20:30:15Z

The missing codecov is from FiltersControl so I'm going to leave it be since it's being removed.

BPScott · 2019-09-30T22:49:17Z

...ResourceList/components/FilterControl/components/FilterValueSelector/FilterValueSelector.tsx

+    [onChange, onFilterKeyChange, value],
+  );
+
+  if (showOperatorOptions && operatorText!.length !== 0 && !isMounted) {


Here we're checking isMounted which is the ref object - I assume it should be isMounted.current - as it stands this will always be false as we're evaluating !{ current: 'someValue'}.

We only ever check isMounted in the render function. If this is being checked in a render function then the mount state is false if it is the first render, or true in every other case. (Compare to being called in a onClick or some other handler callback where the mounted state may change between the handler being called and when we check the current value of the ref)

If I'm interpreting that right then it means that this call to handleOperatorOptionChange only happens after the first render. In that case can we use the existing useIsAfterInitialMount hook instead of creating a new custom hook?

We'd need a useMountedRef hook if we wanted to check the mounted state at any other point where a component may be unmounted - notably in a callback that might wait for a bit of time before being executed (either because of a setInterval or because it was awaiting on some async thing like a network call) and during that time the component got unmounted, but in this case when we only check the value in the render function useIsAfterInitialMount would do the job.

🤦‍♂ I should create a linting rule for this. I'm guessing this'll come up every now and then

This block will execute during the first render, not after. The different between useIsMountedRef & useIsAfterInitialMount is that useIsAfterInitialMount is going to cause a re-render and useIsMountedRef will not.

BPScott · 2019-09-30T23:01:20Z

src/utilities/use-is-mounted-ref.ts

+ * @returns MutableRefObject<boolean> The mounted status
+ */
+export function useIsMountedRef(): MutableRefObject<boolean> {
+  const isMounted = useRef(false);


From http://projects.wojtekmaj.pl/react-lifecycle-methods-diagram/ and https://github.com/donavon/hook-flow it looks like you're right - I guess those "React updates DOM " phases are when something goes from not being mounted to is mounted so in that render before first mount a component is not mounted. I was thrown because the equivalent hook in quilt sets this to true, but it seems like that's a bug on their side: https://github.com/Shopify/quilt/blob/master/packages/react-hooks/src/hooks/mounted-ref.ts

…MountedRef

github-actions · 2019-10-23T17:39:51Z

💦 Potential splash zone of changes introduced to src/**/*.tsx in this pull request:

Files modified	6
Files potentially affected	4

Details

All files potentially affected (total: 4)

📄 UNRELEASED.md (total: 0)

Files potentially affected (total: 0)

🧩 src/components/ResourceList/components/FilterControl/components/FilterValueSelector/FilterValueSelector.tsx (total: 3)

Files potentially affected (total: 3)

🧩 src/components/ResourceList/components/FilterControl/components/FilterValueSelector/index.ts (total: 3)

Files potentially affected (total: 3)

🧩

src/components/ResourceList/components/FilterControl/components/FilterValueSelector/tests/FilterValueSelector.test.tsx (total: 0)

Files potentially affected (total: 0)

🧩 src/utilities/tests/use-is-mounted-ref.test.tsx (total: 0)

Files potentially affected (total: 0)

🧩 src/utilities/use-is-mounted-ref.ts (total: 4)

Files potentially affected (total: 4)

This comment automatically updates as changes are made to this pull request.
Feedback, troubleshooting: open an issue or reach out on Slack in #polaris-tooling.

AndrewMusgrave force-pushed the hookify-fvs branch from aba40ea to 4e43848 Compare September 18, 2019 23:15

AndrewMusgrave commented Sep 18, 2019

View reviewed changes

src/utilities/tests/use-is-mounted-ref.test.tsx Show resolved Hide resolved

AndrewMusgrave marked this pull request as ready for review September 18, 2019 23:19

AndrewMusgrave requested a review from BPScott September 18, 2019 23:19

BPScott mentioned this pull request Sep 25, 2019

Post v4 cleanup work #1995

Closed

BPScott reviewed Sep 25, 2019

View reviewed changes

AndrewMusgrave force-pushed the hookify-fvs branch from 4e43848 to 0c5eedf Compare September 30, 2019 20:29

AndrewMusgrave requested a review from BPScott September 30, 2019 20:30

BPScott reviewed Sep 30, 2019

View reviewed changes

BPScott mentioned this pull request Sep 30, 2019

[useEventListener][useComponentDidMount] Add useComponentDidMount & useEventListener #2203

Merged

6 tasks

AndrewMusgrave requested review from BPScott and dleroux October 3, 2019 19:35

AndrewMusgrave requested review from BPScott and dleroux and removed request for BPScott and dleroux October 21, 2019 16:38

AndrewMusgrave added 3 commits October 23, 2019 13:36

Convert filter value selector to a functional component and add useIs…

7053b50

…MountedRef

hangelog

07bed72

Update

6f71ee5

AndrewMusgrave force-pushed the hookify-fvs branch from 0c5eedf to 6f71ee5 Compare October 23, 2019 17:37

BPScott approved these changes Oct 23, 2019

View reviewed changes

AndrewMusgrave merged commit 7432632 into master Oct 24, 2019

AndrewMusgrave deleted the hookify-fvs branch October 24, 2019 18:48

danrosenthal temporarily deployed to production October 30, 2019 18:54 Inactive

amrocha temporarily deployed to production November 1, 2019 00:13 Inactive

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FilterValueSelector] Convert to functional component & add isMountedRef #2167

[FilterValueSelector] Convert to functional component & add isMountedRef #2167

AndrewMusgrave commented Sep 18, 2019

tmlayton commented Sep 25, 2019

tmlayton commented Sep 25, 2019

BPScott left a comment

BPScott Sep 25, 2019

AndrewMusgrave Sep 30, 2019

BPScott Sep 30, 2019

dleroux Oct 2, 2019

michenly Oct 2, 2019

AndrewMusgrave commented Sep 30, 2019

AndrewMusgrave commented Sep 30, 2019

BPScott Sep 30, 2019

AndrewMusgrave Oct 3, 2019

BPScott Sep 30, 2019

github-actions bot commented Oct 23, 2019 •

edited

Loading

Files potentially affected (total: 0)

Files potentially affected (total: 3)

Files potentially affected (total: 3)

Files potentially affected (total: 0)

Files potentially affected (total: 0)

Files potentially affected (total: 4)

[FilterValueSelector] Convert to functional component & add isMountedRef #2167

[FilterValueSelector] Convert to functional component & add isMountedRef #2167

Conversation

AndrewMusgrave commented Sep 18, 2019

WHY are these changes introduced?

WHAT is this pull request doing?

How to 🎩

🎩 checklist

tmlayton commented Sep 25, 2019

tmlayton commented Sep 25, 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

AndrewMusgrave commented Sep 30, 2019

AndrewMusgrave commented Sep 30, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Oct 23, 2019 • edited Loading

Details

Files potentially affected (total: 0)

Files potentially affected (total: 3)

Files potentially affected (total: 3)

Files potentially affected (total: 0)

Files potentially affected (total: 0)

Files potentially affected (total: 4)

github-actions bot commented Oct 23, 2019 •

edited

Loading