i586 - on click download button it will be disabled for 5 seconds #107
Conversation
ghost
commented
Nov 30, 2017
ghost
commented
- to do this i needed to use local state on DownloadCoverageContentComponent
- to use local state i needed to create proper state interface for this component
- to do this i needed to change parent component RemoteContentComponent to be able to accept state interface
- this change made me change all components using RemoteContentComponent as parent
- now local state interface can be used if needed for all of them
- had to fix some tests
* to do this i needed to use local state on DownloadCoverageContentComponent * to use local state i needed to create proper state interface for this component * to do this i needed to change parent component RemoteContentComponent to be able to accept state interface * this change made me change all components using RemoteContentComponent as parent * now local state interface can be used if needed for all of them * had to fix some tests
ghost
requested review from
|
So far we have been strictly following stateless components, and using stores for state, as per the flux pattern. I think you have more experience with React then me and Alex, Juri, so I'm willing to be convinced, but in general I think that the pure flux pattern is much easier to test. I don't see any tests here? |
| @@ -10,7 +10,7 @@ interface ModellingGroupsProps extends RemoteContent { | |||
| groups: ModellingGroup[] | |||
| } | |||
|
|
|||
| export class ModellingGroupsListComponent extends RemoteContentComponent<ModellingGroupsProps> { | |||
| export class ModellingGroupsListComponent extends RemoteContentComponent<ModellingGroupsProps, {}> { | |||
There was a problem hiding this comment.
In general, we have been using undefined rather than {} in these kind of cases.
There was a problem hiding this comment.
- wrote 2 unit tests: 1 tests on download component and 1 on onetimebutton
- changed {} to undefined
|
FWIW I am totally on board with using local state where appropriate and have also used local state in the burden estimate file upload form, for a v similar purpose (enabling/disabling the submit button.) Just missing tests, but I think the approach makes sense. |
|
I think the top rated comment here makes a lot of sense, re when to use local vs store state! reduxjs/redux#1287 |
|
Okay, fair enough, I'm convinced. In which case, Juri if you just use |
…d on all components inheriting from RemoteContentComponents, * wrote test on setting state to disable button for 5 seconds on responsibility download * wrote test on onetimebutton checking that outercallback is called if passed to this component
| this.setState({ | ||
| downloadButtonEnabled: false, | ||
| }) | ||
| }, 50) |
There was a problem hiding this comment.
I may be wrong about this, but I'll just check: I think the 50 ms delay is unnecessary. If you leave the timing argument off I would expect the browser to finish resolving the user button action, and only when the stack was empty would this callback be invoked.
| this.setState({ | ||
| downloadButtonEnabled: true, | ||
| }) | ||
| }, 5000) |
There was a problem hiding this comment.
Maybe more readable as
setTimeout(() => this.setState({
downloadButtonEnabled: true
}), 5000);
But that's personal preference. Equally the other statement. Also, it might be nice to pull the 5000ms delay into a named static const?
There was a problem hiding this comment.
Although see below about idea to parameterize delay.
| @@ -8,16 +8,17 @@ interface Props { | |||
| token: string | |||
| enabled?: boolean; | |||
| refreshToken: () => void; | |||
| onClickOuterEvent?: () =>void; | |||
There was a problem hiding this comment.
I think this could just be called onClick. Better to have it easy to understand for components using the OneTimeButton. If you'd like to keep things unambiguous, maybe rename the internal method to something like internalOnClickHandler.
| expect(component.state().downloadButtonEnabled).to.be.equal(true) | ||
| done(); | ||
| },5010); | ||
| }); |
There was a problem hiding this comment.
Hmm, this makes this test take 5 seconds to run which is a bit long. Maybe parameterize the delay (via a React prop) so you can test this more rapidly?
* added disabled timeout as parameter * changed test to use 100ms for test of disabling button instead of 5000 * fixed refresh token test * changed makeParams for new structure of params
|
Oh, I just remembered: There's a feature missing here. We also wanted to re-enable the button straight away if the user switches to a different demographic data set or option. If you like, that can be a separate PR. |
|
lets make this feature a separate PR |
|
Opened https://vimc.myjetbrains.com/youtrack/issue/VIMC-1119 for continuing work. |
