-
Notifications
You must be signed in to change notification settings - Fork 7
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 ability to load settings #484
Conversation
@FraserGreenroyd to confirm, the following actions are now queued:
|
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.
After testing, when loading from the given settings file, the settings are correctly set to check ABCDEF and uncheck Acoustic Analytical Addinmanagementsettings, architecture, and audience. When I mess with the settings file itself and provide the popup with invalid json, it correctly returns an error, and doesn't try to apply the settings. The buttons are also easily distinguishable, and as discussed, makes sense to remove the reset button (which didn't actually reset)
@FraserGreenroyd to confirm, the following actions are now queued:
|
@FraserGreenroyd to confirm, the following actions are now queued:
|
@BHoMBot check ready-to-merge |
@FraserGreenroyd to confirm, the following actions are now queued:
|
NOTE: Depends on
BHoM/BHoM_Engine#3300
BHoM/BHoM_Engine#3302
BHoM/BHoM_Engine#3304
Issues addressed by this PR
Fixes #483
Test files
Same as #482
Use this JSON file to load in and see what changes you get:
BH.oM.UI.SearchSettings.json
Try to break the JSON file and check you get errors as well.
You should find the first few alphabetical options are unchecked, and you should get a new addition of
ABCDEF
being checked - it will likely render at the bottom first but on a reload of the settings window you'll find it at the top.Changelog
Additional comments
I have removed the
Reset
button because this was effectively duplicating theSelect/Unselect all
buttons, and it was only resetting to check everything rather than resetting to the originally loaded state or similar. Thus, with discussion with @Tom-Kingstone and @albinber this looked like it could cause a negative UX, so has been removed to avoid confusion.Tooltips have been added to aid usability though.