-
Notifications
You must be signed in to change notification settings - Fork 6
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
Move Audio Hook to new audio package #683
Move Audio Hook to new audio package #683
Conversation
Visit the preview URL for this PR (updated for commit 71ddb8e): https://newm-artist-portal--pr683-create-audio-package-e0v8kl3e.web.app (expires Sun, 14 Jul 2024 09:04:08 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: ec3483d4c62309afc398865bfd6a9fc9e03e1d46 |
Visit the studio preview URL for this PR 🚀 : https://683.artist.preview.newm.io/ |
Visit the tools preview URL for this PR (updated for commit 0c184c2): |
Visit the wallet preview URL for this PR (updated for commit 0c184c2): |
Visit the marketplace preview URL for this PR (updated for commit 0c184c2): |
Visit the tools preview URL for this PR (updated for commit 1153c33): |
Visit the wallet preview URL for this PR (updated for commit 1153c33): |
Visit the marketplace preview URL for this PR (updated for commit 1153c33): |
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.
Had a minor comment but LGTM!
SetAudioProgressAction, | ||
SetAudioUrlAction, | ||
SetIsAudioPlayingAction, | ||
} from "./types"; |
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.
This works, however, another way to create and define the actions in less code is to use createAction
from redux/toolkit
. E.g.
import { createAction } from "@reduxjs/toolkit";
...
const SET_AUDIO = "setAudio"
const setAudio = createAction<Howl | undefined>(SET_AUDIO)
instead of
export interface SetAudioAction {
readonly payload: Howl | undefined;
readonly type: typeof SET_AUDIO;
}
export const setAudio = (payload: AudioState["audio"]): SetAudioAction => {
return {
payload,
type: "setAudio",
};
};
packages/audio/src/lib/reducer.ts
Outdated
case "resetAudioState": | ||
return initialState; | ||
case "setAudio": | ||
return { ...state, audio: action.payload }; |
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.
One thing that can be good to do, just to ensure all of the string values are accurate, is to use the action type variables here. E.g.
import { SET_AUDIO } from "./types";
...
case SET_AUDIO:
return { ...state, audio: action.payload };
Visit the tools preview URL for this PR (updated for commit 559d778): |
Visit the wallet preview URL for this PR (updated for commit 559d778): |
Visit the marketplace preview URL for this PR (updated for commit 559d778): |
519d5c8
into
MRKT-27-Single-Item-for-Play_Update-for-universal-playback
* feat: Add audio module to packages for marketplace * feat: Incorporate audio module into audio hook * fix: Change Marketplace dev port back to original * fix: Add Howl type import to not break build * Move Audio Hook to new audio package (#683) * refactor: Move audio hook to own package * fix: Set port back to initial value * refactor: Improve use of types to reduce mistypes
Moves
usePlayAudioUrl
hook to own package. Refactors state management to utilizeuseContext
anduseReducer
. Overall done to simplify and make hook more modular and self-contained.