Minor refactor #35
Minor refactor #35
Conversation
satya164
force-pushed
the
@satya164/tweaks
branch
from
c60a72e to
e305df5
Compare
satya164
force-pushed
the
@satya164/tweaks
branch
from
e305df5 to
081f1c8
Compare
src/PaletteProvider.js
Outdated
| LoaderComponent?: | ||
| | React$Component<*, *, *> | ||
| | ((...args: *) => React$Element<*>), | ||
| LoaderComponent?: ReactClass<*> | ((...args: *) => React.Element<*>), |
There was a problem hiding this comment.
Let's use Array<*> or Array<any> better for the rest parameter.
From the docs: If you add a type annotation to a rest parameter, it must always explicitly be an Array type.
https://flow.org/en/docs/types/functions/#toc-rest-parameters
| if (onFinish) onFinish(paletteWithDefaults); | ||
|
|
||
| this.setState({ palette: paletteWithDefaults }); | ||
| this.eventEmitter.publish({ |
There was a problem hiding this comment.
We need to publish before setting state, remember the issue we discussed at the office. The change is in my branch anyway, but will be good to have it here as well already 🤖
| return <LoaderComponent />; | ||
| } | ||
|
|
||
| if (shouldRender) { |
There was a problem hiding this comment.
shouldn't we change the order? If forceRender is provided, it seems natural for me to override LoaderComponent, what do you think?
There was a problem hiding this comment.
Just following what's in the comment in the prop types. cc @zamotany
There was a problem hiding this comment.
From the docs: Does not take effect if LoaderComponent is specified!
Besides, why would you specify LoaderComponent when you would force the render anyway?
| try { | ||
| failure(new Error('test')); | ||
| } catch (error) { | ||
| expect(error.message).toMatch(/MaterialPaletteProvider.*test/); |
There was a problem hiding this comment.
Why not expect(error.message).toBe('Uncaught MaterialPaletteProvider exception: test') for more accuracy of what's being thrown?
| lightVibrant: { ...localDefaults.lightVibrant, population: 0 }, | ||
| }); | ||
| expect(style).toEqual([{ fontSize: '14px' }, {}]); | ||
| expect(StyleSheet.flatten(style)).toEqual({ fontSize: '14px' }); |
There was a problem hiding this comment.
Nice! Didn't know about flatten 👍
|
Also, we need to update the API docs to remove |
satya164
force-pushed
the
@satya164/tweaks
branch
from
081f1c8 to
589b7ae
Compare
|
@zamotany it's basically the same as when the provider mounts, so seems unnecessary |
No description provided.