[AppProvider] Update context #1424

AndrewMusgrave · 2019-05-07T21:03:10Z

WHY are these changes introduced?

Sorry for the large pr, a lot of these changes needed to happen at the same time 😅 I tried to keep comments to a minimum since a lot of the stuff repeats.

There was previously an issue with our context types, this required a lot of changes (app bridge, theme, and stickyManager)

A lot of tests needed to change with our new utils. You'll no longer be able to use api's like setState and prop on your mounted component which will promote stronger more valuable tests. Example below

// Bad
const component = mount(<C color="red" />)
expect(c.prop('color')).toBe("red")

// Good
const component = mount(<C color="red" />)
expect(c.find(X).prop('color')).toBe("red")

We no longer have namespaces on Polaris and frame context since we don't have to worry about collisions. This is because we're receiving context from a consumer.

WHAT is this pull request doing?

Moving AppProvider and associated components to the new context api
Removing polaris, frame, theme provider namespace on contexts
Updating context utils
Delete withSticky and moving some logic into withAppProvider
Fix types / tests
Create/update testing utils to work well with the new context

🎩

A lot of the changes are related to tests and types

the currently failing test is related to tsconfig change. (I wonder why it wasn't failing locally? 🤔)

A lovely sticky playground from Dan L.

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

/* eslint-disable */

import * as React from 'react';
import {
  Page,
  ResourceList,
  Card,
  FilterType,
  AppProvider,
  Layout,
  Sticky,
  Modal,
  Button,
} from '@shopify/polaris';
import {autobind} from '@shopify/javascript-utilities/decorators';

interface State {
  searchValue?: string;
  filterSearchFocused: boolean;
  selectedItems: string[] | 'All';
  sortValue?: string;
  openModal: boolean;
}

export default class Playground extends React.Component<never, State> {
  state: State = {
    selectedItems: [],
    filterSearchFocused: false,
    openModal: false,
  };

  render() {
    const {searchValue, openModal} = this.state;

    const resourceName = {
      singular: 'Product',
      plural: 'Products',
    };

    return (
      <AppProvider>
        <Page title="Playground">
          <Layout>
            <Layout.Section>
              <Card>
                <Button onClick={() => this.setState({openModal: true})}>
                  Open Modal
                </Button>
                <ResourceList
                  items={items}
                  renderItem={handleRenderItem}
                  hasMoreItems
                  filterControl={
                    <ResourceList.FilterControl
                      searchValue={searchValue}
                      onSearchChange={this.handleSearchChange}
                      additionalAction={{
                        content: 'Save',
                      }}
                    />
                  }
                  selectedItems={this.state.selectedItems}
                  onSelectionChange={this.handleSelectionChange}
                  promotedBulkActions={[
                    {
                      content: 'Really long text on button 1',
                      onAction: this.bulkActionOne,
                    },
                    {
                      content: 'long text button 2',
                      disabled: true,
                      url: 'http://www.google.com',
                    },
                  ]}
                  bulkActions={[
                    {
                      content: 'button 3',
                      onAction: this.bulkActionThree,
                    },
                    {
                      content: 'button 4',
                      onAction: this.bulkActionFour,
                    },
                    {
                      content: 'button 5',
                      onAction: this.bulkActionFive,
                      disabled: true,
                    },
                  ]}
                  sortValue={this.state.sortValue}
                  sortOptions={mockSortOptions}
                  onSortChange={this.handleSortChange}
                />
                <Modal
                  title="Resource List in card"
                  open={openModal}
                  onClose={() => this.setState({openModal: false})}
                >
                  <ResourceList
                    resourceName={resourceName}
                    items={items}
                    renderItem={handleRenderItem}
                    hasMoreItems
                    filterControl={
                      <ResourceList.FilterControl
                        searchValue={searchValue}
                        onSearchChange={this.handleSearchChange}
                        additionalAction={{
                          content: 'Save',
                        }}
                        filters={mockFilters}
                      />
                    }
                    onSelectionChange={this.handleSelectionChange}
                    sortValue={this.state.sortValue}
                    sortOptions={mockSortOptions}
                    onSortChange={this.handleSortChange}
                  />
                </Modal>
              </Card>
            </Layout.Section>
            <Layout.Section secondary>
              <Sticky offset disableWhenStacked>
                <div style={{paddingBottom: '20px'}}>
                  <Card title="Tags" sectioned>
                    <p>Add tags to your order.</p>
                  </Card>
                </div>
              </Sticky>
              <Card title="Tags" sectioned>
                <p>Add tags to your order.</p>
              </Card>
              <Card title="Tags" sectioned>
                <p>Add tags to your order.</p>
              </Card>
            </Layout.Section>
          </Layout>
        </Page>
      </AppProvider>
    );
  }

  @autobind
  private handleSelectionChange(selectedItems: string[]) {
    this.setState({selectedItems});
  }

  @autobind
  private handleSearchChange(searchValue: string) {
    this.setState({searchValue});
  }

  @autobind
  private handleSortChange(sortValue: string) {
    this.setState({sortValue});
  }

  @autobind
  private bulkActionOne() {
    console.log('Clicked on bulk action one.');
  }

  @autobind
  private bulkActionThree() {
    console.log('Clicked on bulk action three.');
  }

  @autobind
  private bulkActionFour() {
    console.log('Clicked on bulk action four.');
  }

  @autobind
  private bulkActionFive() {
    console.log('Clicked on bulk action five.');
  }
}

function handleRenderItem(item: any, id: any) {
  return (
    <ResourceList.Item
      id={id}
      url={item.url}
      shortcutActions={item.actions}
      accessibilityLabel={`View details for ${item.title}`}
    >
      <div>Item {id}</div>
      <div>{item.title}</div>
    </ResourceList.Item>
  );
}

const items: any[] = [
  {
    onClick: true,
    url: 'https://www.google.com',
    actions: [{content: 'View listing', url: 'http://www.facebook.com'}],
    title: 'Has url, onClick, and actions',
  },
  {
    onClick: false,
    url: 'https://www.google.com',
    actions: [{content: 'View listing', url: 'http://www.facebook.com'}],
    title: 'Has url, and actions',
  },
  {
    onClick: true,
    actions: [{content: 'View listing', url: 'http://www.facebook.com'}],
    title: 'Has onClick, and actions',
  },
  {
    onClick: false,
    url: 'https://www.google.com',
    title: 'Has url',
  },
  {
    onClick: true,
    title: 'Has onClick',
  },
  {
    onClick: true,
    url: 'https://www.google.com',
    actions: [{content: 'View listing', url: 'http://www.facebook.com'}],
    title: 'Has url, onClick, and actions',
  },
  {
    onClick: false,
    url: 'https://www.google.com',
    actions: [{content: 'View listing', url: 'http://www.facebook.com'}],
    title: 'Has url, and actions',
  },
  {
    onClick: true,
    actions: [{content: 'View listing', url: 'http://www.facebook.com'}],
    title: 'Has onClick, and actions',
  },
  {
    onClick: false,
    url: 'https://www.google.com',
    title: 'Has url',
  },
  {
    onClick: true,
    title: 'Has onClick',
  },
  {
    onClick: true,
    url: 'https://www.google.com',
    actions: [{content: 'View listing', url: 'http://www.facebook.com'}],
    title: 'Has url, onClick, and actions',
  },
  {
    onClick: false,
    url: 'https://www.google.com',
    actions: [{content: 'View listing', url: 'http://www.facebook.com'}],
    title: 'Has url, and actions',
  },
  {
    onClick: true,
    actions: [{content: 'View listing', url: 'http://www.facebook.com'}],
    title: 'Has onClick, and actions',
  },
  {
    onClick: false,
    url: 'https://www.google.com',
    title: 'Has url',
  },
  {
    onClick: true,
    title: 'Has onClick',
  },
];

const mockFilters: any[] = [
  {
    key: 'filterKey1',
    label: 'Product type',
    operatorText: 'is',
    type: FilterType.Select,
    options: [
      'Bundle',
      {
        value: 'electronic_value',
        label: 'Electronic',
        disabled: true,
      },
      {
        value: 'beauty_value',
        label: 'Beauty',
      },
    ],
  },
  {
    key: 'filterKey2',
    label: 'Tagged with',
    type: FilterType.TextField,
  },
];

const mockSortOptions = [
  'Product title (A-Z)',
  {
    value: 'PRODUCT_TITLE_DESC',
    label: 'Product title (Z-A)',
  },
  {
    value: 'EXTRA',
    label: 'Disabled Option',
    disabled: true,
  },
];

/* eslint-enable */

Notes

this has a lot of changes so I'll create a changelog entry in a follow-up pr

AndrewMusgrave · 2019-05-07T21:36:18Z

src/components/AppProvider/AppProvider.tsx


 interface State {
-  polaris: Context;
+  context: Context;


Changing this on the local state to be clearer about what it is

AndrewMusgrave · 2019-05-07T21:36:27Z

src/components/AppProvider/types.ts

@@ -31,18 +29,9 @@ export interface AppProviderProps {
 }

 export interface Context {
-  polaris: {


Removing the polaris namespace now since we consume it from a render prop rather than context types. We won't have to worry about collisions now

AndrewMusgrave · 2019-05-07T21:36:38Z

src/components/AppProvider/utilities/StickyManager/StickyManager.ts

@@ -14,7 +14,7 @@ export interface StickyItem {
  /** Placeholder element */
  placeHolderNode: HTMLElement;
  /** Element outlining the fixed position boundaries */
-  boundingElement: HTMLElement | null;
+  boundingElement?: HTMLElement | null;


There was a type issue and boundElement could be undefined

AndrewMusgrave · 2019-05-07T21:36:50Z

src/components/AppProvider/utilities/index.ts

@@ -5,7 +5,7 @@ export {
  PrimitiveReplacementDictionary,
  ComplexReplacementDictionary,
 } from './Intl';
-export {default as withSticky, StickyManager} from './withSticky';


withSticky was deleted

AndrewMusgrave · 2019-05-07T21:36:55Z

src/components/AppProvider/utilities/withAppProvider/tests/withAppProvider.test.tsx

@@ -13,7 +14,11 @@ describe('withAppProvider', () => {
    const WrappedComponent = withAppProvider<any>()(Child);

    const fn = () => {
-      mount(<WrappedComponent />);
+      mount(
+        <AppProviderContext.Provider value={{} as any}>


Wrapping the component in a provider so withAppProvider can consume it

AndrewMusgrave · 2019-05-07T21:37:05Z

src/components/AppProvider/utilities/withAppProvider/withAppProvider.tsx

-    appBridge: ClientApplication<{}>;
-    subscribe(callback: () => void): void;
-    unsubscribe(callback: () => void): void;
+    theme?: ThemeContext;


There was a type issue and theme / appBridge could always be undefined

AndrewMusgrave · 2019-05-07T21:37:11Z

src/components/AppProvider/utilities/withAppProvider/withAppProvider.tsx

-        ? merge(WrappedComponent.contextTypes, polarisAppProviderContextTypes)
-        : polarisAppProviderContextTypes;
+export interface Options {
+  inScrollable?: boolean;


withScrollable | inScrollable? Any opinions?

withinScrollable?

src/components/AppProvider/utilities/withAppProvider/withAppProvider.tsx

AndrewMusgrave · 2019-05-07T21:37:34Z

src/components/AppProvider/utilities/withSticky/withSticky.tsx

@@ -1,55 +0,0 @@
-import * as React from 'react';


A smaller more compact version was included in withAppProvider so we can cut down on individual HoC

AndrewMusgrave · 2019-05-07T21:37:42Z

src/components/Autocomplete/components/ComboBox/tests/ComboBox.test.tsx

@@ -388,21 +388,6 @@ describe('<ComboBox/>', () => {
  });

  describe('keypress events', () => {
-    it('selects the first option when the down arrow is pressed', () => {


This test doesn't do what's expected, the test below covers it.

AndrewMusgrave · 2019-05-07T21:37:47Z

src/components/Autocomplete/components/ComboBox/tests/ComboBox.test.tsx

@@ -430,7 +415,7 @@ describe('<ComboBox/>', () => {
      );

      comboBox.simulate('focus');
-      expect(comboBox.state('popoverActive')).toBe(true);


We should never expect on state (component internals)

src/components/Badge/tests/Badge.test.tsx

src/components/ContextualSaveBar/ContextualSaveBar.tsx

AndrewMusgrave · 2019-05-07T21:38:14Z

src/components/DisplayText/tests/DisplayText.test.tsx

@@ -29,11 +29,4 @@ describe('<DisplayText />', () => {
    );
    expect(displayText.find('p')).toHaveLength(1);
  });
-
-  it('renders the specified size', () => {


We shouldn't expect on the base components props since it doesn't provide value. This test should be handled with visual regression like percy

AndrewMusgrave · 2019-05-07T21:38:28Z

src/components/Layout/tests/Layout.test.tsx

@@ -30,32 +30,6 @@ describe('<Layout />', () => {
    expect(section.find(MyComponent).exists()).toBe(true);
  });

-  describe('<Layout.Section />', () => {


We shouldn't expect on the base components props since it doesn't provide value. These tests should be handled by percy

src/components/Modal/Modal.tsx

AndrewMusgrave · 2019-05-07T21:39:02Z

src/components/Page/Page.tsx

@@ -46,13 +46,16 @@ export class Page extends React.PureComponent<ComposedProps, never> {
  private titlebar: AppBridgeTitleBar.TitleBar | undefined;


Related to the correct appBridge type from context

AndrewMusgrave · 2019-05-07T21:39:17Z

src/components/PositionedOverlay/tests/PositionedOverlay.test.tsx

@@ -36,7 +67,10 @@ describe('<PositionedOverlay />', () => {
      const positionedOverlay = mountWithAppProvider(
        <PositionedOverlay {...mockProps} preferredAlignment="left" />,
      );
-      expect(positionedOverlay.prop('preferredAlignment')).toBe('left');
+
+      expect(


I fixed this test and the test below but we should be relying on visual regression for these

AndrewMusgrave · 2019-05-07T21:39:28Z

src/components/Sticky/Sticky.tsx

@@ -30,8 +30,14 @@ export default class Sticky extends React.Component<Props, State> {
  private stickyNode: HTMLElement | null = null;

  componentDidMount() {
-    const {stickyManager} = this.context.polaris;
-    const {boundingElement, offset, disableWhenStacked} = this.props;
+    const {


The types were originally incorrect and weren't compatible with registerStickyItem so I added defaults and an early return @dleroux

AndrewMusgrave · 2019-05-07T21:39:40Z

src/test-utilities/enzyme.tsx

@@ -27,13 +24,7 @@ export function findByTestID(root: ReactWrapper<any, any>, id: string) {
    return wrapper.length > 0 && wrapper.prop('testID') === id;
  }

-  const rootResult = root.findWhere(hasTestID).first();


Updated this to be compatible with our new testing util

AndrewMusgrave · 2019-05-07T21:39:45Z

src/test-utilities/enzyme.tsx

@@ -82,98 +73,85 @@ export function trigger(wrapper: AnyWrapper, keypath: string, ...args: any[]) {
  return returnValue;
 }

-/**


Removed a few functions below that are no longer in use

AndrewMusgrave · 2019-05-07T21:39:56Z

src/test-utilities/enzyme.tsx

-  }) as any;
+interface MountWithAppProviderOptions {
+  context?: {
+    frame?: DeepPartial<FrameContext>;


DeepPartial is used so we can only merge one branch of an object. For example, updating appBridge from undefined to X but leaving the rest of polaris intact.

AndrewMusgrave · 2019-05-07T21:40:01Z

src/test-utilities/enzyme.tsx

+): AppContextReactWrapper<P, any> {
+  const {context: ctx = {}} = options;
+
+  const polarisDefault = createPolarisContext();


Creating a default and merging it with provided if it exists

AndrewMusgrave · 2019-05-07T21:40:06Z

src/test-utilities/enzyme.tsx

-      // We need this wrapping div because Enzyme expects a single node, not an array.
-      nodeJSX = <div>{nodeJSX}</div>;
-    }
+  const context: AppContext = {


Creating our context to pass too AppContextReactWrapper

AndrewMusgrave · 2019-05-07T21:40:20Z

src/test-utilities/enzyme.tsx

-  options?: ShallowRendererProps,
-): ShallowWrapper<P, any> {
-  return shallow(node, mergeAppProviderOptions(options)).dive(options);
+    this.app = app;


We can now access context, for example

const x = mountWithAppProvider(<Component />) console.log(x.app.polaris.intl.translate(...)

AndrewMusgrave · 2019-05-07T21:40:28Z

src/utilities/merge.ts

+// Unfortunately, this is how we have to type this at the moment.
+// There is currently a proposal to support variadic kinds.
+// https://github.com/Microsoft/TypeScript/issues/5453
+function merge<TSource1, TSource2>(


we needed a stronger type for merge

AndrewMusgrave · 2019-05-07T21:40:36Z

tsconfig.json

@@ -8,7 +8,7 @@
    "resolveJsonModule": true,
    "declaration": true,
    "declarationDir": "types",
-    "jsx": "react-native",
+    "jsx": "react",


Without this change I'm getting ReferenceError: Must call super constructor in derived class before accessing 'this' or returning from derived constructor in tests. If we don't want to make this change could we create a tsconfig for testing or provide some sort of transform to sewing-kit?

Yo! Back in the day we had a thing in our sewing-kit config that force changed the jsx compilation in jest.

It got took out because it didn't seem to be needed. I don't know what in your update stuff has made its return required but it's easy to bring back

296dc34#diff-1afd2752a047671ab9dea43845beb75a

AndrewMusgrave · 2019-05-08T18:52:08Z

src/components/Frame/components/ContextualSaveBar/tests/ContextualSaveBar.test.tsx

-  return mountWithAppProvider(
-    <ThemeProvider value={context}>{node}</ThemeProvider>,
-  );
+function mergeThemeProviderContext(providedThemeContext: ThemeProviderContext) {


This might be overkill, it just saves writing {context: themeProvider: {}}} every time

AndrewMusgrave · 2019-05-08T18:52:32Z

src/components/TopBar/tests/TopBar.test.tsx

@@ -2,13 +2,10 @@ import * as React from 'react';
 import {noop} from '@shopify/javascript-utilities/other';


Using mountWithAppProvider now in this file

AndrewMusgrave · 2019-05-08T18:53:18Z

src/test-utilities/enzyme.tsx

 } from '../components/AppProvider';
 // eslint-disable-next-line shopify/strict-component-boundaries
 import {Provider as FrameProvider, FrameContext} from '../components/Frame';
+// eslint-disable-next-line shopify/strict-component-boundaries


When we export the return value from React.createContext we should be able to import this from '../components'

AndrewMusgrave · 2019-05-08T18:54:29Z

This should be ready for review 🤞

danrosenthal · 2019-05-08T19:32:40Z

Nice one! 🙏

src/components/AppProvider/Context.tsx

danrosenthal

Nicely annotated PR. Thanks for putting in the effort to explain.

I've mostly asked a bunch of questions here to help improve mine and others' understanding of what's going on.

I feel comfortable with merging this once CI is 🍏, especially since it is merging into a feature branch.

We'll want to think about a sane strategy for tophatting the v4 branch with all these changes in place. That probably looks like doing some pre-releasing and testing directly in web.

src/components/AppProvider/Context.tsx

src/components/AppProvider/utilities/createPolarisContext/createPolarisContext.ts

src/components/AppProvider/utilities/createPolarisContext/tests/createPolarisContext.test.tsx

danrosenthal · 2019-05-09T16:23:50Z

src/components/AppProvider/utilities/withAppProvider/withAppProvider.tsx

-        ? merge(WrappedComponent.contextTypes, polarisAppProviderContextTypes)
-        : polarisAppProviderContextTypes;
+export interface Options {
+  inScrollable?: boolean;


withinScrollable?

src/components/AppProvider/utilities/withAppProvider/withAppProvider.tsx

src/components/ScrollLock/ScrollLock.tsx

danrosenthal · 2019-05-09T16:34:33Z

src/components/ScrollLock/tests/ScrollLock.test.tsx

@@ -10,13 +10,18 @@ describe('ScrollLock', () => {
        showScrollLock: true,
      };

+      setScollLockFalse = () => {
+        this.setState({showScrollLock: false});
+      };


Don't we want to avoid setting state in tests?

Changing state within a component through user interactions is ok, however, we want to avoid explicitly setting state with the enzyme API. However, in this case, we may be able to simplify the test by unmounting the wrapper, I'll give that a try.

src/components/Modal/Modal.tsx

src/components/Tabs/tests/Tabs.test.tsx

src/components/TopBar/tests/TopBar.test.tsx

Add types to test until Remove polaris context namespace Remove frame namespace Export createContext result Small quaility change Rename context wrapper Update themeprovider namespace Add type Add theme context to util Update type name for app provider context Update test utility and revert tsconfig changee

BPScott

Looking good.

Nothing jumps out at me, though I'm not very familiar with the AppProvider

Convert ScrollTo Remove dropzone context from index Convert navigation Convert combobox Convert withincontext Update frame context Update resourcelist context Update themeprovider context Update appprovider context Touchups changelog Update appprovider context type Rename file

Update context

[v4] Add usePolaris/AppBridge

BPScott temporarily deployed to polaris-react-pr-1424 May 7, 2019 21:03 Inactive

AndrewMusgrave commented May 7, 2019

View reviewed changes

src/components/AppProvider/utilities/withAppProvider/withAppProvider.tsx Show resolved Hide resolved

AndrewMusgrave commented May 7, 2019

View reviewed changes

src/components/Badge/tests/Badge.test.tsx Show resolved Hide resolved

AndrewMusgrave commented May 7, 2019

View reviewed changes

src/components/ContextualSaveBar/ContextualSaveBar.tsx Show resolved Hide resolved

AndrewMusgrave commented May 7, 2019

View reviewed changes

src/components/Modal/Modal.tsx Show resolved Hide resolved

AndrewMusgrave commented May 7, 2019

View reviewed changes

BPScott temporarily deployed to polaris-react-pr-1424 May 7, 2019 21:43 Inactive

BPScott temporarily deployed to polaris-react-pr-1424 May 7, 2019 21:51 Inactive

AndrewMusgrave commented May 8, 2019

View reviewed changes

AndrewMusgrave added the 🤖Skip Changelog Causes CI to ignore changelog update check. label May 8, 2019

AndrewMusgrave changed the title ~~[WIP][AppProvider] Update context~~ [AppProvider] Update context May 8, 2019

AndrewMusgrave requested review from danrosenthal, dleroux and alex-page May 8, 2019 19:01

alex-page assigned AndrewMusgrave May 8, 2019

AndrewMusgrave marked this pull request as ready for review May 8, 2019 19:10

BPScott temporarily deployed to polaris-react-pr-1424 May 8, 2019 20:21 Inactive

AndrewMusgrave commented May 8, 2019

View reviewed changes

src/components/AppProvider/Context.tsx Outdated Show resolved Hide resolved

danrosenthal approved these changes May 9, 2019

View reviewed changes

BPScott temporarily deployed to polaris-react-pr-1424 May 9, 2019 22:07 Inactive

AndrewMusgrave force-pushed the appprovider-new-context branch from 7f8b15b to 6ee408c Compare May 10, 2019 22:34

BPScott temporarily deployed to polaris-react-pr-1424 May 10, 2019 22:34 Inactive

AndrewMusgrave removed request for dleroux and alex-page May 13, 2019 15:50

BPScott approved these changes May 14, 2019

View reviewed changes

AndrewMusgrave added 2 commits May 14, 2019 10:27

Merge pull request #1462 from Shopify/new-context-style-new-branch

f1565a7

Update context

BPScott temporarily deployed to polaris-react-pr-1424 May 14, 2019 14:43 Inactive

AndrewMusgrave added 2 commits May 14, 2019 11:11

Add hooks

b05b1fe

Merge pull request #1482 from Shopify/create-polaris-hooks

a176047

[v4] Add usePolaris/AppBridge

BPScott deployed to polaris-react-pr-1424 May 15, 2019 16:21 View deployment

AndrewMusgrave merged commit f8ade61 into update-react May 15, 2019

AndrewMusgrave deleted the appprovider-new-context branch May 15, 2019 16:28

		@@ -46,13 +46,16 @@ export class Page extends React.PureComponent<ComposedProps, never> {
		private titlebar: AppBridgeTitleBar.TitleBar \| undefined;

		@@ -2,13 +2,10 @@ import * as React from 'react';
		import {noop} from '@shopify/javascript-utilities/other';

[AppProvider] Update context #1424

[AppProvider] Update context #1424

Conversation

AndrewMusgrave commented May 7, 2019 • edited

WHY are these changes introduced?

WHAT is this pull request doing?

🎩

Notes

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrewMusgrave May 7, 2019 • edited

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

Choose a reason for hiding this comment

AndrewMusgrave commented May 8, 2019

danrosenthal commented May 8, 2019 • edited

danrosenthal 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

BPScott left a comment

Choose a reason for hiding this comment

AndrewMusgrave commented May 7, 2019 •

edited

AndrewMusgrave May 7, 2019 •

edited

danrosenthal commented May 8, 2019 •

edited