-
Notifications
You must be signed in to change notification settings - Fork 8.4k
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 Profile.BellSound to Settings UI #17983
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
This comment has been minimized.
This comment has been minimized.
Probably we don't need to have one browse button per line. Since the indices don't matter in the backend, there is no difference between "replace an entry inline" and "delete old entry, click add (which opens a Browse window)", except that the first is much more complicated from both a developer standpoint (tracking which index to replace) and a user one ("why are there so many buttons?") |
This comment has been minimized.
This comment has been minimized.
Added the demo above. Yeah, for designs 2 & 3, we I think we may be able to remove I'm gonna hold off on making that change for a bit though, because I want consensus on a design first. Adding "Needs-Discussion" so we touch base on the design at the upcoming sync. |
Feedback from team sync (10/7):
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
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.
Just a couple of questions
{ | ||
newSounds.Append(sound); | ||
} | ||
_profile.BellSound(newSounds); |
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.
Will this cause the inherited sounds to be written to the json for this profile even if no edits are made to them?
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.
Nope. From testing it just now:
- clicking save when no changes have been made --> bell sound isn't added
- clicking save after adding a sound --> sound gets serialized
- clicking save after adding a sound them deleting the added sound --> bell sound serialized as empty list
NOTE: I did find a bug where adding a sound when none are defined in the inheritance tree would cause a crash! That'll be fixed in the upcoming commit.
Summary of the Pull Request
Adds the Profile.BellSound setting to the Settings UI under the Profile > Advanced page.
CurrentBellSounds
keeps track of the bell sounds added and exposed via the UI.BellSoundViewModel
wraps each sound. This allows us to listen (and propagate) changes to the registered sounds.BellSoundPreview
provides a written preview of the current bell sound to display in the expander#10000