[DateSelector] Hookify component #2193

AndrewMusgrave · 2019-09-24T21:20:55Z

WHY are these changes introduced?

Part of #1995

WHAT is this pull request doing?

Convert DateSelector to a functional component

Notes

I noticed while converting this component,the setState callback was used quite often however I couldn't figure out why, except when setting state with the date. I'm adding @dleroux since he'll have the most context on this

How to 🎩

Playground with date filter

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: 'accountStatusFilter',
        label: 'Date selector',
        type: FilterType.DateSelector,
        minKey: '0',
        maxKey: '30',
      },
    ];

    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

BPScott

A few small inline comments.

I've not ran this and am unfamilar with the DateSelector component so I'd appreciated a pair of eyes from someone who knows this component

BPScott · 2019-09-25T22:47:54Z

src/components/ResourceList/components/FilterControl/components/DateSelector/DateSelector.tsx

-    datePickerYear: this.now.getFullYear(),
-    initialConsumerFilterKey: this.props.filterKey,
-  };
+// eslint-disable-next-line react/display-name


Can we reference the upstream bug, like we do in ContextualSaveBar

// This does have a display name, but the linting has a bug in it // https://github.com/yannickcr/eslint-plugin-react/issues/2324 // eslint-disable-next-line react/display-name

src/components/ResourceList/components/FilterControl/components/DateSelector/DateSelector.tsx

BPScott · 2019-09-25T23:05:02Z

src/components/ResourceList/components/FilterControl/components/DateSelector/DateSelector.tsx

+}: DateSelectorProps) {
+  const now = new Date();
+
+  const {translate} = useI18n();


Existing code tends to do const i18n = useI18n(); (my personal preference) or const intl = useI18n(); (a hangover from before we renamed this) rather than destructure.

We should stick to one style throughout the codebase. Which one do you prefer?

I would definitely prefer i18n or intl, the name clashing seems odd, best to just stick with one. However, we usually deconstruct whenever possible in our code. So I'm wondering if we should use const {translate} = useI18n() to be consistent? I'm wondering if the reason we didn't originally deconstruct was that iirc web was used as a guideline and they can't deconstruct because they need access to the context, however, we don't but we just didn't deconstruct? 🤷‍♂ Either way as long as well consistent I'm ok with it.

i18n is an instance of the I18n class, so I'm kinda surprised that we can even destructure it in the first place. I thought destructuring only worked on objects.

To reduce others being suprised by that I'd be tempted to avoid destructuring and stick with const i18n = useI18n().

i18n is an instance of the I18n class, so I'm kinda surprised that we can even destructure it in the first place. I thought destructuring only worked on objects.

In JavaScript, mostly everything is going to be an object, even regex since it's not primitive. Classes themselves are not objects, but functions (typeof class Test {} === 'function') however they create a instance of the function in an object (typeof new Test === 'object'). The problem with destructuring is that it'll remove context.

class Test { constructor() { this.num = 5; } func() { return this.num; } } // All good const t = new Test(); t.func(); // Losing context const {func} = t; func(); // Bad error ☹️

We don't use this within I18n so we're ok deconstructing.

To reduce others being suprised by that I'd be tempted to avoid destructuring and stick with const i18n = useI18n().

Two more points to support both sides of the argument.

Unless your playing with i18n you won't know it originated from a class.

If we don't deconstruct we'll be able to add features that do use this within having to refactor.

Huh neat! It seems like I18n's translate function already uses this so by that logic this shouldn't even work now. And yet it seems to manage. That still feels like it's more by luck than judgement though, so I'd continue to prefer not destructuring

Oops looks like we are. It's working because translate is class property (not currently part of JavaScript but in the works)

class Test { func = () => {} }

Could compile down to something like

class Test { constructor() { this.func = () => {} } }

Lamda functions lack certain contexts, including this and usually have the environment they were created in.

class Test { func = () => { console.log(this) } } const {func} = new Test; func(); // All ok

class Test { func() { console.log(this) } } // Sorta gross alternative without using class properties still deconstructing const test = new Test; const {func} = test; const boundFunc = func.bind(test); boundFunc(); // All ok

Either way, I'm ok with not deconstructing our classes, it'll help future proof us and reduce the change of errors. We should reach out for other opinions as well.

Talked it over with @dleroux and we're all on board with not deconstructing

github-actions · 2019-10-03T18:59:49Z

Results

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

Files modified	4
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/DateSelector/DateSelector.tsx (total: 4)

Files potentially affected (total: 4)

🧩 src/components/ResourceList/components/FilterControl/components/DateSelector/index.ts (total: 4)

Files potentially affected (total: 4)

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

Files potentially affected (total: 0)

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.

BPScott

Nice one! I've not tested too thoroughly as I'm not that familiar with DateSelector but LGTM.

AndrewMusgrave changed the title ~~[WIP] Hookify DateSelector~~ [DateSelector] Hookify component Sep 24, 2019

AndrewMusgrave marked this pull request as ready for review September 24, 2019 21:27

BPScott mentioned this pull request Sep 25, 2019

Post v4 cleanup work #1995

Closed

danrosenthal requested a review from BPScott September 25, 2019 15:04

BPScott reviewed Sep 25, 2019

View reviewed changes

AndrewMusgrave added 2 commits October 3, 2019 14:55

Hookify DateSelector

51f969e

changelog

cfab98f

AndrewMusgrave force-pushed the hookify-date-selector branch from 5398c4d to cfab98f Compare October 3, 2019 18:57

AndrewMusgrave requested a review from BPScott October 3, 2019 18:58

BPScott approved these changes Oct 10, 2019

View reviewed changes

AndrewMusgrave merged commit 6e29a84 into master Oct 10, 2019

AndrewMusgrave deleted the hookify-date-selector branch October 10, 2019 15:17

tmlayton temporarily deployed to production October 16, 2019 00:32 Inactive

[DateSelector] Hookify component #2193

[DateSelector] Hookify component #2193

Uh oh!

Conversation

AndrewMusgrave commented Sep 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

WHY are these changes introduced?

WHAT is this pull request doing?

Notes

How to 🎩

🎩 checklist

Uh oh!

BPScott left a comment

Choose a reason for hiding this comment

Uh oh!

BPScott Sep 25, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

BPScott Sep 25, 2019

Choose a reason for hiding this comment

Uh oh!

AndrewMusgrave Sep 26, 2019

Choose a reason for hiding this comment

Uh oh!

BPScott Sep 26, 2019

Choose a reason for hiding this comment

Uh oh!

AndrewMusgrave Sep 30, 2019

Choose a reason for hiding this comment

Uh oh!

BPScott Sep 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AndrewMusgrave Oct 1, 2019

Choose a reason for hiding this comment

Uh oh!

AndrewMusgrave Oct 1, 2019

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Oct 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Results

Details

Files potentially affected (total: 0)

Files potentially affected (total: 4)

Files potentially affected (total: 4)

Files potentially affected (total: 0)

Uh oh!

BPScott left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

AndrewMusgrave commented Sep 24, 2019 •

edited

Loading

BPScott Sep 30, 2019 •

edited

Loading

github-actions bot commented Oct 3, 2019 •

edited

Loading

BPScott left a comment •

edited

Loading