-
Notifications
You must be signed in to change notification settings - Fork 137
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
Added Reset Buttons for every Settings Screen #992
base: redesign
Are you sure you want to change the base?
Conversation
Thanks for the PR! Changes look good, here are a few comments:
|
Oh and I just noticed you already added a button for resetting everything! That would've been my next step. Awesome! |
Just tested it out, everything seems to work. I have one more suggestion: |
…he reset buttons with dialog
Instead of a new Class I made a function in the settings helper, kinda against convention but i felt like its cleaner than having a new class.
And of course app localization needs to be added for the 4 new strings that came with the function, dont know how though |
Thanks! I mean it's not like we need to save on components, so maybe a regular widget is better :) For the state reloading, you can simply pass an anonymous function to FinampSettingsHelper.makeSettingsResetButtonWithDialog(
context,
() {
setState(() {
FinampSettingsHelper.resetAlbumSettings();
// optionally assign a new key for child widgets here
});
},
) That should work just fine. As for localizations, you go into |
I believe thats everything done you asked for :D |
Fyi: Some already existing update functions I used dont update the UI, dont know how to fix that
Also idk why
pubspec.lock
got updated, i guess thats normal?Please double check the defaults ive put there, I believe they are the real defaults since I copied the settings I had which I didnt change much if at all