-
Notifications
You must be signed in to change notification settings - Fork 148
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
Implemented new feature for enable audio , created options sound for All or only errors #41
Conversation
SaurabPoudel
commented
Oct 8, 2023
- But there is a warning : The AudioContext was not allowed to start. It must be resumed (or created) after a user gesture on the page. I couldn't fix that
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.
Thanks for your PR!
I found a few issues in it. Let's fix those in the first round of review.
enableSoundStyle: EnableSoundStyle.All, | ||
whitespaceStyle: WhitespaceStyle.Bullet, | ||
sounds: false, |
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.
Let's remove these two properties and merge them into one.
So, remove the enableSourndsStyle
and replace sounds
with
playSounds: PlaySounds.None,
sounds: booleanProp("textDisplay.sounds", false), | ||
enableSoundStyle: enumProp( | ||
"textDisplay.enableSoundStyle", | ||
EnableSoundStyle, | ||
EnableSoundStyle.All, | ||
), |
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.
Make these properties match the type definitions above.
playSounds: enumProp("textDisplay.playSounds", PlaySounds, PlaySounds.None),
@@ -38,6 +38,7 @@ export function toTextInputSettings(settings: Settings): TextInputSettings { | |||
export type TextDisplaySettings = { | |||
readonly caretShapeStyle: CaretShapeStyle; | |||
readonly caretMovementStyle: CaretMovementStyle; | |||
readonly enableSoundStyle: EnableSoundStyle; | |||
readonly whitespaceStyle: WhitespaceStyle; | |||
readonly sounds: boolean; |
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.
readonly playSounds: PlaySounds;
} as const; | ||
|
||
export function toTextDisplaySettings(settings: Settings): TextDisplaySettings { | ||
return { | ||
caretShapeStyle: settings.get(textDisplayProps.caretShapeStyle), | ||
caretMovementStyle: settings.get(textDisplayProps.caretMovementStyle), | ||
enableSoundStyle: settings.get(textDisplayProps.enableSoundStyle), |
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.
Please keep the order of properties match the order in the type definition.
@@ -49,7 +50,7 @@ export const Controller = memo(function Controller({ | |||
function usePracticeState(state: PracticeState) { | |||
const keyboard = useKeyboard(); | |||
const [lines, setLines] = useState(state.lines); // Forces ui update. | |||
|
|||
const { settings } = useSettings(); |
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.
A big problem here. You use the settings
variableinside a hook, but this variable is not marked as a hook dependency. You should catch this error if you run linter
npm run lint`.
playSound(TextInputSound.Blip); | ||
break; | ||
function playFeedbackSound(feedback: Feedback, option: number): void { | ||
if (option === 1) { |
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.
Please don't use magic numbers. Use PlaySounds.None
, PlaySounds.All
, etc.
Hey, I've made my own change-list to implement this feature -- https://github.com/aradzie/keybr.com/tree/sounds |
Hey Saurab. I know you wanted to contribute, but this is a simple change and it takes equal time to review your PR and to implement this feature. So I did it myself in this commit Again, I am sorry I did not gave you the opportunity to contribute. I know the feeling of excitement when you work is recognized. Maybe next time. |
@aradzie haha cool |