Implement missing-info warnings #1004
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1004 +/- ##
==========================================
+ Coverage 68.84% 68.92% +0.07%
==========================================
Files 115 115
Lines 6417 6443 +26
==========================================
+ Hits 4418 4441 +23
- Misses 1999 2002 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
Very good! |
edan-bainglass
force-pushed
the
missing-info-warnings
branch
from
1ae8749 to
870bd0c
Compare
superstar54
reviewed
Dec 20, 2024
Comment on lines
+156
to
+149
| if step is self.configure_step and not self.structure_model.confirmed: | ||
| step.show_missing_information_warning() | ||
| elif step is self.submit_step and not self.configure_model.confirmed: | ||
| step.show_missing_information_warning() | ||
| elif step is self.results_step and not self.submit_model.confirmed: | ||
| step.show_missing_information_warning() |
There was a problem hiding this comment.
Instead of checking it manually, I think we can do this in an automatic manner: one step depends on its previous step.
There was a problem hiding this comment.
Yeah, probably. But maybe we can do that in another PR? Low on time at the moment. Let's try to get this in for the release and open an issue to revisit this.
superstar54
approved these changes
Dec 20, 2024
edan-bainglass
force-pushed
the
missing-info-warnings
branch
from
870bd0c to
e4d87d8
Compare
edan-bainglass
added a commit
that referenced
this pull request
Dec 24, 2024
…1032) This PR resolves issues with #1004 (incorrect handling of step-to-step switching/rendering) by treating the app steps more uniformly. It also introduces a simple locking mechanism on submission that disconnects all observations of step confirmation, thus rendering the app in a "locked" state by permanently disabling confirm/submit buttons.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR introduces guards on subsequent steps that inform the user if dependencies have yet to be met, i.e., previous step(s) not confirmed.
Pros
Cons
*Not exactly true. The exploration is still available, but now in a prescribed (correct) order.
@cpignedoli @giovannipizzi thoughts?