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

Improve handling of revert button enabled state #314

Merged

Conversation

mwcraig
Copy link
Contributor

@mwcraig mwcraig commented May 1, 2024

This PR fixes a mistake I think made in #310. in handling whether the save and revert buttons in the button bar were enabled.

In that PR I made it so that the save and revert buttons were enabled only if:

  • there were unsaved changes AND
  • the widget held a valid pydanitc model.

That makes sense for the save button -- you should only be able to save a valid model.

Requiring the widget hold a valid pydantic for the revert button makes little sense. One of the reasons you might want to revert, after all, is that you have accidentally entered one or more invalid fields.

This PR changes the behavior for the revert button so that it is enabled whenever there are unsaved changes.

@JuanCab and @Tanner728 -- a look at the code would be great, but confirmation that this new logic makes sense would be enough...I'm reasonably confident the code does what it should.

@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 73.86%. Comparing base (e0155c0) to head (d901d65).
Report is 9 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #314      +/-   ##
==========================================
+ Coverage   73.80%   73.86%   +0.05%     
==========================================
  Files          27       27              
  Lines        3402     3409       +7     
==========================================
+ Hits         2511     2518       +7     
  Misses        891      891              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@Tanner728 Tanner728 left a comment

Choose a reason for hiding this comment

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

All of the code looks good to me, although I don't have much experience with UI stuff. I did some quick testing of it and all of it seems to work as expected. I know we had talked about the revert button earlier but have forgotten what you had decided to do with it as it still does nothing. Just to let you know.

@mwcraig
Copy link
Contributor Author

mwcraig commented May 1, 2024

it still does nothing. Just to let you know.

A PR to fix that is coming, but I needed to address enabling the revert button first (and am trying to keep the PRs smaller and easier to review)

Copy link
Contributor

@JuanCab JuanCab left a comment

Choose a reason for hiding this comment

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

The code looks good. I also looked at the notebook, specifically:

I executed the following commands:

conda create -n test-314 python=3.11 astroscrappy
conda activate test-314
pip install git+https://github.com/mwcraig/stellarphot.git@improve-default-revert-enabled

and then tried the saveable-settings.ipynb notebook. It behaved as advertised (as far as I could tell through creating one entry and then changing it).

@mwcraig mwcraig merged commit 630d07a into feder-observatory:main May 1, 2024
9 checks passed
@mwcraig mwcraig deleted the improve-default-revert-enabled branch May 1, 2024 18:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants