-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
i586 - on click download button it will be disabled for 5 seconds #107
Changes from 2 commits
f435f8e
ca0b3d7
66647f6
2b97eea
9e4bd6a
98fedba
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -27,7 +27,19 @@ interface Props extends HasFormatOption { | |
coverageToken: string; | ||
} | ||
|
||
export class DownloadCoverageContentComponent extends RemoteContentComponent<DownloadCoverageComponentProps> { | ||
interface DownloadState { | ||
downloadButtonEnabled: boolean; | ||
} | ||
|
||
export class DownloadCoverageContentComponent extends RemoteContentComponent<DownloadCoverageComponentProps, DownloadState> { | ||
constructor() { | ||
super(); | ||
this.state = { | ||
downloadButtonEnabled: true | ||
} | ||
this.onDownloadClicked = this.onDownloadClicked.bind(this); | ||
} | ||
|
||
static getStores() { | ||
return [responsibilityStore]; | ||
} | ||
|
@@ -43,7 +55,7 @@ export class DownloadCoverageContentComponent extends RemoteContentComponent<Dow | |
scenario: r.scenario, | ||
coverageSets: r.coverageSets, | ||
coverageToken: state.coverageOneTimeToken, | ||
selectedFormat: state.selectedFormat | ||
selectedFormat: state.selectedFormat, | ||
} | ||
}; | ||
} else { | ||
|
@@ -54,7 +66,6 @@ export class DownloadCoverageContentComponent extends RemoteContentComponent<Dow | |
} | ||
} | ||
|
||
|
||
onSelectFormat(format: string) { | ||
coverageSetActions.selectFormat(format); | ||
responsibilityStore.fetchOneTimeCoverageToken().catch(doNothing); | ||
|
@@ -65,6 +76,19 @@ export class DownloadCoverageContentComponent extends RemoteContentComponent<Dow | |
responsibilityStore.fetchOneTimeCoverageToken(); | ||
} | ||
|
||
onDownloadClicked() { | ||
setTimeout(() => { | ||
this.setState({ | ||
downloadButtonEnabled: false, | ||
}) | ||
}, 50) | ||
setTimeout(() => { | ||
this.setState({ | ||
downloadButtonEnabled: true, | ||
}) | ||
}, 5000) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe more readable as
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Although see below about idea to parameterize delay. |
||
} | ||
|
||
renderContent(props: DownloadCoverageComponentProps) { | ||
const data = props.props; | ||
return <div> | ||
|
@@ -132,7 +156,12 @@ export class DownloadCoverageContentComponent extends RemoteContentComponent<Dow | |
</div> | ||
</div> | ||
<div className="mt-4"> | ||
<OneTimeButton token={data.coverageToken} refreshToken={this.refreshToken}> | ||
<OneTimeButton | ||
token={data.coverageToken} | ||
refreshToken={this.refreshToken} | ||
enabled={this.state.downloadButtonEnabled} | ||
onClickOuterEvent={this.onDownloadClicked} | ||
> | ||
Download combined coverage set data in CSV format | ||
</OneTimeButton> | ||
</div> | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,16 +8,17 @@ interface Props { | |
token: string | ||
enabled?: boolean; | ||
refreshToken: () => void; | ||
onClickOuterEvent?: () =>void; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this could just be called |
||
} | ||
|
||
export class OneTimeButton extends React.Component<Props, undefined> { | ||
export class OneTimeButton extends React.Component<Props, any> { | ||
static defaultProps: Partial<Props> = { | ||
enabled: true | ||
}; | ||
|
||
constructor() { | ||
super(); | ||
this.refreshToken = this.refreshToken.bind(this); | ||
this.onClick = this.onClick.bind(this); | ||
} | ||
|
||
refreshToken() { | ||
|
@@ -32,14 +33,23 @@ export class OneTimeButton extends React.Component<Props, undefined> { | |
} | ||
} | ||
|
||
onClick() { | ||
this.refreshToken(); | ||
if (typeof this.props.onClickOuterEvent === 'function') { | ||
this.props.onClickOuterEvent(); | ||
} | ||
} | ||
|
||
render() { | ||
const props = this.props; | ||
const url = fetcher.fetcher.buildOneTimeLink(props.token); | ||
const enabled = props.enabled && props.token != null; | ||
return <form action={ url }> | ||
<button onClick={ this.refreshToken } | ||
disabled={ !enabled } | ||
type="submit"> | ||
<button | ||
onClick={ this.onClick } | ||
disabled={ !enabled } | ||
type="submit" | ||
> | ||
{ this.props.children } | ||
</button> | ||
{ this.renderAnimation() } | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,7 +1,7 @@ | ||
import * as React from "react"; | ||
import { expect } from "chai"; | ||
import { mockCoverageSet, mockScenario, mockTouchstone } from "../../../mocks/mockModels"; | ||
import { shallow, ShallowWrapper } from "enzyme"; | ||
import { shallow, ShallowWrapper, mount } from "enzyme"; | ||
|
||
import { | ||
DownloadCoverageContentComponent, | ||
|
@@ -45,6 +45,7 @@ describe("DownloadCoverageContentComponent", () => { | |
expect(rendered.find(OneTimeButton).props()).to.eql({ | ||
token: "TOKEN", | ||
refreshToken: (rendered.instance() as DownloadCoverageContentComponent).refreshToken, | ||
onClickOuterEvent: (rendered.instance() as DownloadCoverageContentComponent).onDownloadClicked, | ||
enabled: true, | ||
children: "Download combined coverage set data in CSV format" | ||
}); | ||
|
@@ -58,6 +59,22 @@ describe("DownloadCoverageContentComponent", () => { | |
expect(fetchNewToken.called).to.be.true; | ||
}); | ||
|
||
it("calling meth onDownloadClicked sets state prop downloadButtonEnabled to false for 5 seconds", function(done: DoneCallback) { | ||
this.timeout(5020); | ||
const props = makeProps({ coverageToken: "TOKEN" }); | ||
const component = shallow(<DownloadCoverageContentComponent {...props} />); | ||
const instance = component.instance() as DownloadCoverageContentComponent; | ||
expect(component.state().downloadButtonEnabled).to.be.equal(true) | ||
instance.onDownloadClicked(); | ||
setTimeout(() => { | ||
expect(component.state().downloadButtonEnabled).to.be.equal(false) | ||
},70); | ||
setTimeout(() => { | ||
expect(component.state().downloadButtonEnabled).to.be.equal(true) | ||
done(); | ||
},5010); | ||
}); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? |
||
|
||
it("renders format control", () => { | ||
const props = makeProps({ selectedFormat: "x" }); | ||
const rendered = shallow(<DownloadCoverageContentComponent {...props} />); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.