Skip to content
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

Add warning dialog in LGV before returning to import form to prevent accidentally losing the current view #1774

Merged
merged 2 commits into from Mar 10, 2021

Conversation

cmdcolin
Copy link
Collaborator

@cmdcolin cmdcolin commented Mar 8, 2021

Currently there is a "return to import form" helper on the linear genome view, however if it is used accidentally you can lose your current view

This adds a dialog prompt to check if you are sure

It creates a model state for tracking whether a dialog is open

@github-actions github-actions bot added the needs label triage Needs a label to show in changelog (breaking, enhancement, bug, documentation, or internal) label Mar 8, 2021
@codecov
Copy link

codecov bot commented Mar 8, 2021

Codecov Report

Merging #1774 (d517701) into master (40fa55c) will decrease coverage by 0.05%.
The diff coverage is 11.53%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1774      +/-   ##
==========================================
- Coverage   58.70%   58.65%   -0.06%     
==========================================
  Files         456      457       +1     
  Lines       21170    21191      +21     
  Branches     5011     5017       +6     
==========================================
+ Hits        12428    12429       +1     
- Misses       8433     8452      +19     
- Partials      309      310       +1     
Impacted Files Coverage Δ
...GenomeView/components/ReturnToImportFormDialog.tsx 7.14% <7.14%> (ø)
...s/linear-genome-view/src/LinearGenomeView/index.ts 83.42% <12.50%> (-0.63%) ⬇️
...c/LinearGenomeView/components/LinearGenomeView.tsx 74.35% <25.00%> (-5.65%) ⬇️

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 40fa55c...d517701. Read the comment docs.

@cmdcolin cmdcolin requested a review from rbuels March 8, 2021 21:45
@cmdcolin cmdcolin removed the needs label triage Needs a label to show in changelog (breaking, enhancement, bug, documentation, or internal) label Mar 8, 2021
@rbuels rbuels changed the title Add warning before returning to import form Add warning dialog in LGV before returning to import form to prevent accidentally losing the current view Mar 9, 2021
@rbuels rbuels added the enhancement New feature or request label Mar 9, 2021
Copy link
Contributor

@rbuels rbuels left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is ok to merge, just a couple of suggestions for possible improvements

Comment on lines 74 to 82
onClick={() => {
model.setDisplayedRegions([])
// it is necessary to run these after setting displayed regions
// empty or else model.offsetPx gets set to infinity and breaks
// mobx-state-tree snapshot
model.scrollTo(0)
model.zoomTo(10)
handleClose()
}}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

might be a good idea to move this code into a new action in the model in order to keep the react code focused just on presentation

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Created this as the "clearView" action

@@ -439,6 +442,10 @@ export function stateModelFactory(pluginManager: PluginManager) {
},
}))
.actions(self => ({
// eslint-disable-next-line @typescript-eslint/no-explicit-any
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any way we could reduce the number of instances of any?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did a couple changes where I manually tell the dialog that it receives an object with a clearView function. It's not really possible to type the ReturnToImportFormDialog as receiving a full LGV prop I believe without making it any here

@cmdcolin cmdcolin merged commit 663179b into master Mar 10, 2021
@cmdcolin cmdcolin deleted the warn_return_to_importform branch March 10, 2021 01:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants