[typescript] Constrain props type param appropriately in withStyles, withTheme, withWidth HOCs #11003

estaub · 2018-04-12T13:34:58Z

UPDATED
The current structure of withStyles, withTheme, and withWidth forces use of a generic parameter to describe the component's properties without the injected properties.

This changes the generic parameter to reflect the entire props of the inner component, so that it can be inferred from the passed component.

My experience is that the current `withStyles` declaration works poorly with Typescript 2.8. I'm not sure that it's the compiler version that's the issue, though. At least in my usage, it fails to remove `WithStyles<ClassKey>` from the return type. This definition works better in my experience, doesn't rely on subtle inferencer behavior in the same way, and seems to be in the mainstream of current usage. There's a built-in `Exclude` function in Typescript 2.8 that does what `Omit` does. I used `Omit` for compatibility with older compilers.

estaub · 2018-04-12T16:35:17Z

@pelotom Tom, if you've a few cycles, could you look at this before I try to fix all the downstream issues? You may have tried this approach and didn't get good results, and/or may have concerns about breaking changes (ahem ;-).

pelotom

Many variations on this signature have been tried and abandoned because of one pain point or another. If you fix the tests we can see what it looks like and evaluate whether it's better on balance.

pelotom · 2018-04-12T16:50:53Z

src/styles/withStyles.d.ts

@@ -38,9 +38,13 @@ export interface StyledComponentProps<ClassKey extends string = string> {
  innerRef?: React.Ref<any>;
 }

+// Diff / Omit taken from https://github.com/Microsoft/TypeScript/issues/12215#issuecomment-311923766
+export type Diff<T extends string, U extends string> = ({ [P in T]: P } & { [P in U]: never } & { [x: string]: never })[T];
+export type Omit<T, K extends keyof T> = Pick<T, Diff<keyof T, K>>;


FYI, Diff and Omit are already defined in index.d.ts:

https://github.com/mui-org/material-ui/blob/29cbe2c8c1fbb1ac7341ffd6e499a56f6030bf86/src/index.d.ts#L43-L47

pelotom · 2018-04-12T17:02:31Z

src/styles/withStyles.d.ts

+    style: StyleRules<ClassKey> | StyleRulesCallback<ClassKey>,
+    options?: WithStylesOptions,
+): <P extends WithStyles<ClassKey>>(
+    component: React.ComponentType<P>,


Extraneous indentation (use yarn prettier to clean this up).

pelotom · 2018-04-12T21:07:54Z

So in all use cases within the test code you have increased the notational burden with this change. How is it supposed to be better again?

estaub · 2018-04-12T23:09:05Z

I'll write up an issue with examples and link this to it.
I believe the current definition doesn't work correctly, not doing the Omit at all; you need to see that.

In my own usage, I create and expect a type definition at hand for the entire set of props that a component expects, including those injected by HOCs, so the "increased notational burden" doesn't exist in my small universe. But of course, other folks' usage ~~may~~ does differ!

Some other HOC declarations, such as those for react-router:

export function withRouter<P extends RouteComponentProps<any>>(component: React.ComponentType<P>): React.ComponentClass<Omit<P, keyof RouteComponentProps<any>>>;

require such a type as a generic input. If the component is well-typed, the generic is inferred. So in the current case, this would work:

const Decoratee: React.SFC<Props & WithStyles<'root'>> = ({ text, variant, color, classes }) => (
  <Typography variant={variant} color={color} classes={classes}>
    {text}
  </Typography>
)
const DecoratedSFC = decorate(Decoratee)

If this PR is otherwise a good idea (which needs to be evaluated in the context of the issue-to-be-written), I should probably update the tests to reflect this style of writing.

estaub · 2018-04-13T00:05:25Z

@pelotom I won't be submitting an issue after all; I just figured out the nature and source of the issue. See the updated initial comment, above.

pelotom · 2018-04-13T00:10:40Z

UPDATED
This is intended to fix an issue that was caused by #10880

That PR wasn't merged, so how could it be "causing" anything?

estaub · 2018-04-13T00:22:29Z

Oy - yeah. I'm not sure what made this become evident. The rest stands, though, IMHO.

Obviously, it's a breaking change. For folks who use typed components, it means deleting the generic parameter, which will probably be welcomed. For others, not so much!

pelotom · 2018-04-13T00:32:35Z

Sorry, I'm still not seeing the benefit. Please provide a complete, self-contained example showing what it looks like with the current typing and how your proposed typing improves it.

estaub · 2018-04-13T01:31:08Z

I've updated the tests so that they make the case; if they don't float your boat, probably nothing will, and it's time to close this.

At the risk of belaboring the obvious: declaring the properties and their types of a component has independent value in helping in IDE and compiler catch errors that otherwise will only be detected at runtime. Dragging people into doing this may cause a little grumbling, but I think it'll be less than the grumbling from needlessly requiring a generic parameter.

I started this thinking I was seeing a bug; I just assumed that one would design these HOC declarations so that they would work well with inferred typing. I can't begin to guess how common that expectation is; it may be just me.

estaub · 2018-04-13T13:25:24Z

@oliviertassinari fwiw, Argos appears to be stuck. No urgency here!

oliviertassinari · 2018-04-13T13:27:13Z

@estaub Yeah, Argos-CI is down. I have a weird out of memory issue.

estaub · 2018-04-13T13:45:45Z

@pelotom When you get to reviewing this, I'd suggest that you look at the full-text (not just diff) of styles.spec.ts. The diff makes it hard to see the gist of it.

oliviertassinari · 2018-04-14T14:53:59Z

@estaub Argos-CI is back to normal ✅.

estaub · 2018-04-14T16:31:14Z

attempting to get tests to rerun...

pelotom · 2018-04-14T17:56:21Z

src/styles/withStyles.d.ts

-) => React.ComponentType<P & StyledComponentProps<ClassKey>>;
+): <P extends WithStyles<ClassKey>>(
+  component: React.ComponentType<P>,
+) => React.ComponentType<Omit<P, 'classes'> & StyledComponentProps<ClassKey>>;


I think there's some merit here, but P should not be required to included a classes prop. Instead it should be required that if P does include a classes prop, it should have the right type. Something like this:

interface MaybeWithStyles extends Partial<WithStyles<ClassKey>> { [k: string]: any; } export default function withStyles<ClassKey extends string>( style: StyleRules<ClassKey> | StyleRulesCallback<ClassKey>, options?: WithStylesOptions, ): <P extends MaybeWithStyles<ClassKey>>( component: React.ComponentType<P & WithStyles<ClassKey>>, ) => React.ComponentType<Omit<P, 'classes'> & StyledComponentProps<ClassKey>>;

That looks great! Let me check it out a bit.

estaub · 2018-04-14T20:49:56Z

That commit went well - the only changes in the tests, from the original, are now the (optional) removals of generic parameters.

pelotom · 2018-04-14T21:03:51Z

src/utils/withWidth.d.ts

-) => React.ComponentClass<P & Partial<WithWidthProps>>;
+): <P extends WithWidthProps>(
+  component: React.ComponentType<P>,
+) => React.ComponentClass<Omit<P, keyof WithWidthProps> & Partial<WithWidthProps>>;


Similarly, this shouldn't require P to contain width:

export default function withWidth( options?: WithWidthOptions, ): <P extends Partial<WithWidthProps> & Record<string, any>>( component: React.ComponentType<P & WithWidthProps>, ) => React.ComponentClass<Omit<P, keyof WithWidthProps> & Partial<WithWidthProps>>;

With all of these HOCs we want to enable this kind of pattern:

const Component = withWidth()<{ // shouldn't need to specify width here; it's a given name: string }>(({ width, name }) => <div style={{ width }}>hello, {name}</div> );

Yeah, I wondered about those two, should have hit them. I'll add a couple of tests, too.

pelotom · 2018-04-14T21:16:11Z

src/styles/withTheme.d.ts

-) => React.ComponentClass<P>;
+declare const withTheme: () => <P extends WithTheme>(
+  component: React.ComponentType<P>,
+) => React.ComponentClass<Omit<P, keyof WithTheme>>;


Similarly, this shouldn't require P to contain theme...

pelotom · 2018-04-14T21:20:06Z

I'm pretty sure this means both of the "2 scenarios" mentioned in https://material-ui.com/guides/typescript/ about having to specify explicit type parameters can be removed now. It would be good to do that as part of this PR.

pelotom · 2018-04-14T21:21:10Z

src/styles/withStyles.d.ts

+): <P extends WithStyles<ClassKey>>(
+  component: React.ComponentType<P>,
+) => React.ComponentType<Omit<P, 'classes'> & StyledComponentProps<ClassKey>>;
+*/


Let's get rid of this instead of commenting out.

pelotom · 2018-04-14T21:26:05Z

src/styles/withStyles.d.ts

+
+interface MaybeWithStyles<ClassKey extends string> extends Partial<WithStyles<ClassKey>> {
+  [k: string]: any;
+}


How about making a reusable version of this,

type ConsistentWith<O> = Partial<O> & Record<string, any>;

which can then be used for withStyles, withWidth and withTheme:

P extends ConsistentWith<WithStyles<ClassKey>> P extends ConsistentWith<WithWidthProps> P extends ConsistentWith<WithTheme>

Remove commented-on code from withStyles; use ConsistentWith typewrapper; add SFC test for withWidth add FAILING test for doc scenario 2 (withStyles of union)

estaub · 2018-04-16T14:20:58Z

Surprisingly at this point, I've reached a hard blocker with this.

The "DecoratedUnionProps" or "Scenario 2" example on https://material-ui.com/guides/typescript/ didn't have a test, so I threw one into styling-comparison.spec.tsx, and it threw back. It's committed in the patch, for your review.

The "Omit" implementation fails to retain the alternative properties in a union. In the example, for "ArtProps" it retains"category" but not "artist" or "author". I didn't find any way that providing the current generic parameter explicitly helps, and doubt there would be any.

If you have any ideas, I'm all ears twice over! I did some experimenting with the new 2.8 Exclude, and it also fails, with a different message, complaining that the union is not an interface.

Here are the options I can see:

Bail out - just close the PR.
Fix it!!! Though I'm not seeing how. I've thought about raising an issue at TypeScript, asking if anyone has an Omit that works for this, but doubt it'd be fruitful.
Bail on support of top-level unions; require them to be wrapped in a non-union object. I have no clear sense of how palatable this is, but think about the trade-off of correct inferencing in other cases. A working example is included at the end of styling-comparison.spec.tsx.
Provide a different way to supply a generic parameter containing just the "ownProps" type, as the current implementation does. I'm being deliberately vague, as my only ideas for this aren't very good.

I didn't check in any doc changes, pending where we go.

estaub · 2018-04-16T23:22:04Z

I don't know what's going on, but the ci behavior was very strange too, first failing, then succeeding. So chances are others would get the behavior you're seeing.
Any idea what could be going on?

I rolled back that test change and doc change; AFAIK, this PR is ready to merge.

Tangentially... where did you learn the multi-signature syntax you used to fix withStyles? I've never seen it before. More generally, I feel like my ability to learn how to do good definitions is limited by not knowing where to look for deep reference material - am I missing something? I find the spec pretty impenetrable, but it's the only resource I'm aware of for something like this.

pelotom · 2018-04-16T23:31:59Z

I don't know what's going on, but the ci behavior was very strange too, first failing, then succeeding. So chances are others would get the behavior you're seeing.
Any idea what could be going on?

I rolled back that test change and doc change; AFAIK, this PR is ready to merge.

I think you're confusing yourself somehow. The union type argument has always been there in the test, you never removed it. Your last commit says

Bring back explicit generic parameter on union case in test, doc.

but if you look at the actual diff nothing changed in the test.

Tangentially... where did you learn the multi-signature syntax you used to fix withStyles? I've never seen it before. More generally, I feel like my ability to learn how to do good definitions is limited by not knowing where to look for deep reference material - am I missing something? I find the spec pretty impenetrable, but it's the only resource I'm aware of for something like this.

It's called "overloading", and it's been around since the very early days of TypeScript. All I can say is, you will learn over time with practice. I wish they still maintained an up-to-date spec, but they don't 🤷‍♂️

pelotom · 2018-04-16T23:35:40Z

package.json

@@ -46,7 +46,7 @@
    "test:coverage:html": "cross-env NODE_ENV=test BABEL_ENV=coverage nyc mocha test/**/*.test.js src/{,**/}*.test.js && nyc report --reporter=html",
    "test:karma": "cross-env NODE_ENV=test karma start test/karma.conf.js --single-run",
    "test:regressions": "webpack --config test/regressions/webpack.config.js && rimraf test/regressions/screenshots/chrome/* && vrtest run --config test/vrtest.config.js --record",
-    "typescript": "tsc -p tsconfig.json",
+    "typescript": "tsc -v && tsc -p tsconfig.json",


Please revert.

pelotom · 2018-04-16T23:35:45Z

package.json

@@ -187,7 +187,7 @@
    "rimraf": "^2.6.2",
    "sinon": "^4.1.2",
    "size-limit": "0.14.1",
-    "typescript": "^2.6.1",
+    "typescript": "2.6.2",


Please revert.

Ok, but again, it doesn't compile in 2.6.1. Do you want me to remove the fragment from the test?

That doesn't matter, the yarn.lock is what controls the actual version this project depends on, and it did point to 2.8.1 until you changed it. There should be no changes to either package.json or yarn.lock in this PR.

pelotom · 2018-04-16T23:35:51Z

test/typescript/styling-comparison.spec.tsx

@@ -68,11 +68,11 @@ const DecoratedUnionProps = decorate<ArtProps>( // <-- without the type argument
      const props = this.props;
      return (
        <Typography classes={props.classes}>
-          {props.category === 'book' ? props.author : props.artist}
+          {props.category === "book" ? props.author : props.artist}


Please revert.

Ok, done, but what is the intended preference?
Both are used in that file, roughly 50/50

My immediate preference is to keep irrelevant changes out of this PR 🙂

pelotom · 2018-04-16T23:35:55Z

test/typescript/styling-comparison.spec.tsx

        </Typography>
      );
    }
-  },
+  }


Please revert.

pelotom · 2018-04-16T23:36:00Z

yarn.lock

-  resolved "https://registry.yarnpkg.com/typescript/-/typescript-2.8.1.tgz#6160e4f8f195d5ba81d4876f9c0cc1fbc0820624"
+[email protected].2:
+  version "2.6.2"
+  resolved "https://registry.yarnpkg.com/typescript/-/typescript-2.6.2.tgz#3c5b6fd7f6de0914269027f03c0946758f7673a4"


Please revert.

pelotom · 2018-04-16T23:36:43Z

docs/src/pages/guides/typescript/typescript.md

@@ -8,7 +8,7 @@ Have a look at the [Create React App with TypeScript](https://github.com/mui-org
 The usage of `withStyles` in TypeScript can be a little tricky, so it's worth showing some examples. You can first call `withStyles()` to create a decorator function, like so:

 ```js
-const decorate = withStyles(({ palette, spacing }) => ({
+const withMyStyles = withStyles(({ palette, spacing }) => ({


Please don't rename decorate.

Use single quotes in test. In package.json, revert typescript to 2.6.1, remove version-printing run command In package.lock, revert typescript to 2.6.1.

estaub · 2018-04-17T00:06:06Z

What do you want me to on the merge?
Do you want me to try to get most of it into the patch, or are you going to handle it?

pelotom · 2018-04-17T00:07:52Z

docs/src/pages/guides/typescript/typescript.md

-
-Scenario 2: `Props` is a union type. Again, to avoid getting a compiler error, you'll need to provide an explict type argument:
+When your `props` are a union, Typescript needs you to explicitly tell it the type, 
+by providing an explicit generic `<Props>` parameter to `decorate`:


Remove newline

pelotom · 2018-04-17T00:08:22Z

package.json

@@ -187,7 +187,7 @@
    "rimraf": "^2.6.2",
    "sinon": "^4.1.2",
    "size-limit": "0.14.1",
-    "typescript": "^2.6.1",
+    "typescript": "2.6.1",


This still needs to be reverted.

pelotom · 2018-04-17T00:09:31Z

There are some conflicts with v1-beta that need to be fixed.

pelotom · 2018-04-17T00:16:18Z

What do you want me to on the merge?
Do you want me to try to get most of it into the patch, or are you going to handle it?

I'm not sure what you're asking, but you need to merge v1-beta into this branch before I can merge the PR.

pelotom · 2018-04-17T00:17:46Z

src/styles/withStyles.d.ts

@@ -1,4 +1,6 @@
 import * as React from 'react';
+import { WithTheme } from '../styles/withTheme';
+import { Omit, ConsistentWith } from '..';


The Omit import is no longer necessary so it should be removed

pelotom · 2018-04-17T00:25:47Z

src/styles/withStyles.d.ts

+  <P extends ConsistentWith<WithStyles<ClassKey>>>(
+    component: React.ComponentType<P & WithStyles<ClassKey>>,
+  ): React.ComponentType<P & StyledComponentProps<ClassKey>>;
+}


Missing semicolon

pelotom · 2018-04-17T00:28:08Z

docs/src/pages/guides/typescript/typescript.md

-```
-
-Scenario 2: `Props` is a union type. Again, to avoid getting a compiler error, you'll need to provide an explict type argument:
+When your `props` are a union, Typescript needs you to explicitly tell it the type, by providing an explicit generic `<Props>` parameter to `decorate`:


-by providing an explicit generic +by providing a generic

Remove the "explicit", it's implicit that it's explicit from the preceding "explicitly" :)

pelotom · 2018-04-17T00:50:38Z

test/typescript/styling-comparison.spec.tsx

@@ -46,3 +46,33 @@ const DecoratedNoProps = decorate<{}>(
 const sfcElem = <DecoratedSFC text="Hello, World!" variant="title" color="secondary" />;
 const classElem = <DecoratedClass text="Hello, World!" variant="title" color="secondary" />;
 const noPropsElem = <DecoratedNoProps />;
+
+// This is the "scenario 2" example straight from the doc, then invoked:


There is no scenario 2 now, maybe just provide a link or else delete the comment.

… patch-1

pelotom

LGTM 👍

pelotom · 2018-04-17T01:38:18Z

Thanks, it was a long slog but worth it!

estaub · 2018-04-17T03:13:12Z

Back at you!

estaub · 2018-04-28T15:29:02Z

@pelotom I didn't want to spam the subscribers on # 11109 with implementation discussion, so this is here.

The earlier contravariance writeup got me thinking backward about a conditional typing approach to this, and you may well be right that there's an answer there. This 2.8-dependent withStyles declaration fails the AllTheComposition in styles.spec.tsx, but otherwise seems to work. It uses a technique I haven't seen elsewhere to test whether a generic subtype is the same as the parent, by testing whether the parent extends the subtype:

(WithStyles<ClassKey> extends P? P: never)

export default function withStyles<ClassKey extends string>(
  style: StyleRules<ClassKey> | StyleRulesCallback<ClassKey>,
  options?: WithStylesOptions,
): {
  <P extends WithStyles<ClassKey>>(
    component: React.ComponentType<P & (WithStyles<ClassKey> extends P? P: never)>):
    React.ComponentType<{_1?:never} & StyledComponentProps<ClassKey>>;

  <P extends ConsistentWith<WithStyles<ClassKey>>>(
    component: React.ComponentType<P & WithStyles<ClassKey>>,
  ): React.ComponentType<{_2?:never} & P & StyledComponentProps<ClassKey>>;
};

I'm afraid I'm going to have to bail on this now, or at least very soon; it's been a few orders of magnitude more time than I ever imagined. If you come to think a rollback is in order, just do it.

franklixuefei · 2018-05-01T02:03:08Z

still reports error for #11191

pelotom suggested changes Apr 12, 2018

View reviewed changes

oliviertassinari added the typescript label Apr 12, 2018

Fix test issues; redesign withTheme, withWidth using Omit also.

f488adf

estaub changed the title ~~Rewrite withStyles Typescript declaration, using Omit~~ Rewrite withStyles, withTheme, withWidth Typescript declarations, using Omit Apr 12, 2018

estaub changed the title ~~Rewrite withStyles, withTheme, withWidth Typescript declarations, using Omit~~ Use Omit for withStyles, withTheme, withWidth Typescript declarations Apr 12, 2018

Use preferred style in tests

3560ac0

Remove another inferrable generic parameter from test.

8eb517a

Remove another inferrable generic parameter from test.

175fa49

pelotom suggested changes Apr 14, 2018

View reviewed changes

Make inclusion of WithStyles in the innerComponent's type def optional.

92bee1b

pelotom suggested changes Apr 14, 2018

View reviewed changes

Commit for review and suggestions; DOES NOT COMPILE.

c01d787

Remove commented-on code from withStyles; use ConsistentWith typewrapper; add SFC test for withWidth add FAILING test for doc scenario 2 (withStyles of union)

pelotom suggested changes Apr 16, 2018

View reviewed changes

estaub added 2 commits April 16, 2018 19:58

Rename withMyStyles back to decorate.

9fbbbc1

Use single quotes in test. In package.json, revert typescript to 2.6.1, remove version-printing run command In package.lock, revert typescript to 2.6.1.

Add trailing comma to function params in styling-comparison test.

e66b8ea

pelotom suggested changes Apr 17, 2018

View reviewed changes

estaub added 2 commits April 16, 2018 20:10

Fix package.json, yarn.lock.

0b47fb3

Remove newline from typescript doc.

748af39

pelotom suggested changes Apr 17, 2018

View reviewed changes

Remove needless Omit imports.

b24250b

pelotom suggested changes Apr 17, 2018

View reviewed changes

Remove extra explicit from doc, add trailing semicolon to function

ac934c1

pelotom suggested changes Apr 17, 2018

View reviewed changes

estaub added 2 commits April 16, 2018 20:51

Merge branch 'v1-beta' of https://github.com/mui-org/material-ui into…

f1dbf16

… patch-1

Remove comment from styling-comparison.spec.tsx

ea20df9

pelotom approved these changes Apr 17, 2018

View reviewed changes

pelotom merged commit 0e86314 into mui:v1-beta Apr 17, 2018

emreeren mentioned this pull request Apr 23, 2018

Question regarding latest (beta.43) WithStyles update. #11109

Closed

1 task

estaub deleted the patch-1 branch April 23, 2018 23:25

franklixuefei mentioned this pull request May 1, 2018

[TypeScript] withStyles still cannot infer generic type from its parameter #11191

Closed

1 task

estaub mentioned this pull request May 3, 2018

Allow Exclude when the difference contains a union. microsoft/TypeScript#23862

Closed

pelotom mentioned this pull request Jul 11, 2018

Better types for withStyles WIP - no merge yet - #12106

Closed

[typescript] Constrain props type param appropriately in withStyles, withTheme, withWidth HOCs #11003

[typescript] Constrain props type param appropriately in withStyles, withTheme, withWidth HOCs #11003

Conversation

estaub commented Apr 12, 2018 • edited Loading

estaub commented Apr 12, 2018

pelotom left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pelotom commented Apr 12, 2018

estaub commented Apr 12, 2018 • edited Loading

estaub commented Apr 13, 2018

pelotom commented Apr 13, 2018 • edited Loading

estaub commented Apr 13, 2018

pelotom commented Apr 13, 2018

estaub commented Apr 13, 2018 • edited Loading

estaub commented Apr 13, 2018

oliviertassinari commented Apr 13, 2018 • edited Loading

estaub commented Apr 13, 2018

oliviertassinari commented Apr 14, 2018

estaub commented Apr 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

estaub commented Apr 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pelotom commented Apr 14, 2018 • edited by zannager Loading

Choose a reason for hiding this comment

pelotom Apr 14, 2018 • edited Loading

Choose a reason for hiding this comment

estaub commented Apr 16, 2018 • edited by zannager Loading

estaub commented Apr 16, 2018

pelotom commented Apr 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

estaub Apr 16, 2018 • 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

estaub commented Apr 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pelotom commented Apr 17, 2018 • edited Loading

pelotom commented Apr 17, 2018

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

pelotom left a comment

Choose a reason for hiding this comment

pelotom commented Apr 17, 2018

estaub commented Apr 17, 2018

estaub commented Apr 28, 2018

franklixuefei commented May 1, 2018

estaub commented Apr 12, 2018 •

edited

Loading

estaub commented Apr 12, 2018 •

edited

Loading

pelotom commented Apr 13, 2018 •

edited

Loading

estaub commented Apr 13, 2018 •

edited

Loading

oliviertassinari commented Apr 13, 2018 •

edited

Loading

pelotom commented Apr 14, 2018 •

edited by zannager

Loading

pelotom Apr 14, 2018 •

edited

Loading

estaub commented Apr 16, 2018 •

edited by zannager

Loading

estaub Apr 16, 2018 •

edited

Loading

pelotom commented Apr 17, 2018 •

edited

Loading