-
-
Notifications
You must be signed in to change notification settings - Fork 13
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
Port Pronunciations to use redux-toolkit #2753
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅ see 1 file with indirect coverage changes 📢 Thoughts on this report? Let us know! |
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.
Reviewed 7 of 7 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @imnasnainaec)
src/components/Pronunciations/Redux/PronunciationsActions.ts
line 19 at r1 (raw file):
} export function reset(): Action {
This is easily confused with the global reset action. I recommend changing it to a more specific name, e.g. resetPronunciation()
Code quote:
reset()
src/components/Pronunciations/Redux/PronunciationsReduxTypes.ts
line 2 at r1 (raw file):
export enum PronunciationsStatus { Default = "DEFAULT",
I don't think Default
is a good name for the default status. It does not tell me anything about what the status is - it just tells me that its the status when the app is started up. I end up assuming that it is neither playing nor recording. It would be better to have something like:
export enum PronunciationsStatus {
Inactive = "INACTIVE",
Playing = "PLAYING",
Recording = "RECORDING",
}
and set defaultState.status
to be PronunciationsStatus.Inactive
.
Code quote:
Default = "DEFAULT",
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.
Reviewed 5 of 5 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @imnasnainaec)
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.
Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @imnasnainaec)
Part of #1953
This change is