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

componentize warning dialog #2918

Merged
merged 1 commit into from Oct 11, 2017

Conversation

Projects
None yet
5 participants
@meandavejustice
Member

meandavejustice commented Oct 9, 2017

  • moved warning dialog out of view component
  • now renders in app.js
  • also componetized view.js
  • added moved tests to upgradewarningpage dir and added stories

@meandavejustice meandavejustice requested a review from fzzzy Oct 9, 2017

@lmorchard

This comment has been minimized.

Show comment
Hide comment
@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Oct 9, 2017

Codecov Report

Merging #2918 into master will decrease coverage by 13.04%.
The diff coverage is 94.69%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master    #2918       +/-   ##
===========================================
- Coverage   94.88%   81.83%   -13.05%     
===========================================
  Files          86      107       +21     
  Lines        2267     2670      +403     
===========================================
+ Hits         2151     2185       +34     
- Misses        116      485      +369
Impacted Files Coverage Δ
...end/src/app/containers/UpgradeWarningPage/index.js 100% <100%> (ø)
frontend/src/app/components/View/index.js 100% <100%> (ø)
...end/src/app/containers/UpgradeWarningPage/tests.js 100% <100%> (ø)
frontend/src/app/containers/App/tests.js 100% <100%> (ø)
frontend/src/app/components/View/tests.js 100% <100%> (ø)
frontend/test/app/containers/RestartPage-test.js 100% <100%> (ø) ⬆️
frontend/src/app/containers/App/index.js 7.37% <30%> (ø)
frontend/src/app/components/MainInstallButton.js 84.37% <0%> (ø) ⬆️
...rontend/src/app/containers/ExperimentPage/index.js
...ntainers/ExperimentPage/ExperimentDisableDialog.js
... and 37 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e7e8856...82b210a. Read the comment docs.

codecov-io commented Oct 9, 2017

Codecov Report

Merging #2918 into master will decrease coverage by 13.04%.
The diff coverage is 94.69%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master    #2918       +/-   ##
===========================================
- Coverage   94.88%   81.83%   -13.05%     
===========================================
  Files          86      107       +21     
  Lines        2267     2670      +403     
===========================================
+ Hits         2151     2185       +34     
- Misses        116      485      +369
Impacted Files Coverage Δ
...end/src/app/containers/UpgradeWarningPage/index.js 100% <100%> (ø)
frontend/src/app/components/View/index.js 100% <100%> (ø)
...end/src/app/containers/UpgradeWarningPage/tests.js 100% <100%> (ø)
frontend/src/app/containers/App/tests.js 100% <100%> (ø)
frontend/src/app/components/View/tests.js 100% <100%> (ø)
frontend/test/app/containers/RestartPage-test.js 100% <100%> (ø) ⬆️
frontend/src/app/containers/App/index.js 7.37% <30%> (ø)
frontend/src/app/components/MainInstallButton.js 84.37% <0%> (ø) ⬆️
...rontend/src/app/containers/ExperimentPage/index.js
...ntainers/ExperimentPage/ExperimentDisableDialog.js
... and 37 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e7e8856...82b210a. Read the comment docs.

@lmorchard

Tests pass, looks good. Just had a couple of nits - mainly need to add some more stories to cover the warning states.

@@ -189,16 +207,14 @@ function sendToGA(type, dataIn) {
const mapStateToProps = state => ({

This comment has been minimized.

@fzzzy

fzzzy Oct 10, 2017

Collaborator

At some point soon we should split up this monolithic function and have one for every container.

Not in this pull request though.

@fzzzy

fzzzy Oct 10, 2017

Collaborator

At some point soon we should split up this monolithic function and have one for every container.

Not in this pull request though.

This comment has been minimized.

@lmorchard

lmorchard Oct 10, 2017

Member

Maybe file an issue toward that? I think the majority of these are shared across containers thanks to common subcomponents, so that could use some homework to figure out how true that might be

@lmorchard

lmorchard Oct 10, 2017

Member

Maybe file an issue toward that? I think the majority of these are shared across containers thanks to common subcomponents, so that could use some homework to figure out how true that might be

This comment has been minimized.

@fzzzy

fzzzy Oct 10, 2017

Collaborator

Filed:

#2924

Maybe it won't be worth it, but we should at least investigate it.

@fzzzy

fzzzy Oct 10, 2017

Collaborator

Filed:

#2924

Maybe it won't be worth it, but we should at least investigate it.

@fzzzy

This comment has been minimized.

Show comment
Hide comment
@fzzzy

fzzzy Oct 10, 2017

Collaborator

This looks good, with Les' changes.

Collaborator

fzzzy commented Oct 10, 2017

This looks good, with Les' changes.

componentize warning dialog
- moved warning dialog out of view component
- now renders in app.js
- also componetized view.js
- added moved tests to upgradewarningpage dir and added stories
@meandavejustice

This comment has been minimized.

Show comment
Hide comment
@meandavejustice

meandavejustice Oct 11, 2017

Member

fixed up those changes, this is much better for testing, thanks for the suggestions @lmorchard

Member

meandavejustice commented Oct 11, 2017

fixed up those changes, this is much better for testing, thanks for the suggestions @lmorchard

@lmorchard

This comment has been minimized.

Show comment
Hide comment
@lmorchard

lmorchard Oct 11, 2017

Member

Okay, I think we've beat this one up enough, looks good to me!

Member

lmorchard commented Oct 11, 2017

Okay, I think we've beat this one up enough, looks good to me!

@lmorchard lmorchard merged commit 9d829ba into master Oct 11, 2017

1 check passed

ci/circleci Your tests passed on CircleCI!
Details

@meandavejustice meandavejustice deleted the 2778-warning-dialog-improvements branch Oct 11, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment