-
-
Notifications
You must be signed in to change notification settings - Fork 826
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
Don't store checkbox instances in NotificationGrid #2183
Conversation
…g in Checkbox and Switch
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.
I'm fine with this approach.
I do think we should also take care of your other remarks at the same time.
In particular the preferenceSaver
thing WILL need change anyway to work with Mithril 2. We will need accompanying PRs for the extensions that use this method.
@clarkwinkelmann Can you expand on that? It seems to contradict what @askvortsov1 said in the PR description:
I'm probably just misunderstanding something, though. |
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.
Looks good, but Clark's comments need to be taken care of.
(It's sad that the checkboxes don't completely encapsulate their loading state, but that's been a problem before, so it's not relevant here...)
The Currently the way that instance is obtained is through the second parameter of An alternative could be to pass the |
Argh that's a good point, I hadn't thought of that... Considering that pretty much every preference is updated on change, perhaps preferenceLoadingState could be separated out as a property of the current user along with the preferences themselves? |
I've been thinking about this, but can't come up with a good place to store the preference loading states:
My favorite here is probably 2, but I would also be alright with 3. Not a fan of 1 or 4. |
@askvortsov1 yes my thought was option 2.
I don't think that's necessary. It's already a wrapper around the very apt I'd suggest applying my two proposed code samples from the in-code comments and then I'm ok with the PR. |
Breaking changes:
|
Refs #1821 #2144
Changes proposed in this pull request:
Reviewers should focus on:
view
is very repetetive. Can we somehow put the key into a variable?init
of SettingsPage, but that would still require extensions that add preferences to include a reference to that loadingState, breaking their loading indicators)Confirmed
composer test
).