MiniPlayer : Added Mini player component. #7
Conversation
VizbeeMiniCastBar/helpers.js
Outdated
| * @returns {string} title to display | ||
| */ | ||
| export const getTitleFromVideoInfo = ({ title, castingTo }) => { | ||
| if (title && title.length) { |
There was a problem hiding this comment.
Use optional chaining operator e.g., title?.length
…-vizbee-sender-sdk into feature/mini_player
… blank mini player issue
index.js
Outdated
| export { default as VizbeeTrackInfo } from "./VizbeeTrackInfo"; | ||
| export const VizbeeCastButton = requireNativeComponent("VizbeeCastButtonView"); | ||
| export { default as VizbeeMiniCastBar } from "./src/mini-cast-bar"; |
There was a problem hiding this comment.
let's move it to the "components" folder, something like this "./src/components/mini-cast-bar"
There was a problem hiding this comment.
Also, The convention widely adopted is PascalCase (UpperCamelCase) for component names and folder structures. Can you recheck on this rename to VizbeeMiniCastBar?
There was a problem hiding this comment.
I see that now it's exported as export { VizbeeMiniCastBar } which enforces using the specified name when other modules want to import it. I think we should give flexibility to other modules in choosing a name during the import
src/mini-cast-bar/index.js
Outdated
| const percentage = (streamPosition / streamDuration) * 100; | ||
|
|
||
|
|
||
| return isVisible ? ( |
There was a problem hiding this comment.
Can we move this functional component to a .js file something like VZBMiniCastBarComponent.js as It's not a common best practice to write a functional component directly in the index.js file of a React application?
src
|-- components
| |-- mini-cast-bar
| |-- hooks
| |-- index.js
| |-- useMiniPlayerStyleHook
| |-- ...
| |-- index.js
| |-- VZBMiniCastBarComponent.js
src/mini-cast-bar/hooks/index.js
Outdated
| @@ -0,0 +1,6 @@ | |||
|
|
|||
| export {useMiniPlayerMediaStatusListenerHook} from "./useMiniPlayerMediaStatusListenerHook" | |||
There was a problem hiding this comment.
I know the broad architecture we are following here is the functional components with hooks and for new development, it's often recommended to use functional components with hooks.
The only concern I have is that both hooks and functional components were introduced in RN 0.59 but the peerDependencies that we added in our react-native-wrapper package is 0.41.2.
- Should we update our
peerDependenciesto 0.59? - If so, any existing customers who are on <0.59 are forced to update to 0.59, can we validate this by preparing list of customer who are using our react-native wrapper and what version of react-native they are on?
There was a problem hiding this comment.
Whom should i ask for point 2?
There was a problem hiding this comment.
Let's go with the following list for now
Tegna - we will get it
NFL > 0.59
NewsON > 0.59
|
|
||
| public void setAppTheme(@StyleRes int theme) { | ||
| VizbeeContext.getInstance().setTheme(theme); | ||
| } |
There was a problem hiding this comment.
Revert this change if it's only used in the demo app. Apps can directly call
VizbeeContext.getInstance().setTheme(theme); in their java files. Add vizbee-sender-sdk as a dependency to the apps module if necessary.
There was a problem hiding this comment.
Are you sure apps has access to VizbeeContext because i didn't have access to it from the wrapper.
There was a problem hiding this comment.
Apps have to add the dependency of native SDK
src/mini-cast-bar/constants.js
Outdated
| STOP: "stop", | ||
| }; | ||
|
|
||
| export const LOG_TAG = "MiniPlayer" |
There was a problem hiding this comment.
Change log tag to VZBRNSDK_MiniCastBar
| @@ -0,0 +1,36 @@ | |||
| import { VizbeeManager } from "../../../"; | |||
| import { ButtonType } from "../constants"; | |||
| import { LOG_TAG } from "../constants"; | |||
There was a problem hiding this comment.
let's update the import to import { ButtonType, LOG_TAG } from "../constants";
| default: | ||
| break; | ||
| } | ||
| }; |
There was a problem hiding this comment.
Is there a need for outer function? can't it be written simply like the following?
const onButtonPress = (buttonType) => {
console.debug(LOG_TAG, `Miniplayer control pressed :: ${buttonType}.`);
switch (buttonType) {
case ButtonType.PLAY:
VizbeeManager.play();
break;
case ButtonType.PAUSE:
VizbeeManager.pause();
break;
case ButtonType.STOP:
VizbeeManager.stop();
break;
default:
break;
}
};
| * Called on Miniplayer View press | ||
| */ | ||
| const onMiniPlayerViewPress = () => { | ||
| console.debug(LOG_TAG, `Miniplayer View pressed`); |
There was a problem hiding this comment.
Use template literals consistently
console.debug(`${LOG_TAG} Miniplayer View pressed`);
| */ | ||
| const onButtonPress = (buttonType) => | ||
| function () { | ||
| console.debug(LOG_TAG, `Miniplayer control pressed :: ${buttonType}.`); |
There was a problem hiding this comment.
Use template literals consistently
console.debug(`${LOG_TAG} Miniplayer control pressed :: ${buttonType}.`);
| import { ButtonType } from "../constants"; | ||
| import { LOG_TAG } from "../constants"; | ||
|
|
||
| export const useMiniPlayerActionsHook = () => { |
There was a problem hiding this comment.
Can you add comments to explain the purpose of the hook and the main functions. Make sure you add the same for all the hooks.
| VizbeePlayerState, | ||
| } from "../../../"; | ||
| import { ButtonType } from "../constants"; | ||
| import { LOG_TAG } from "../constants"; |
| return; | ||
| } | ||
|
|
||
| if (newMediaStatus?.playerState === VizbeePlayerState.Paused || newMediaStatus?.playerState === VizbeePlayerState.Loading || newMediaStatus?.playerState === VizbeePlayerState.Buffering) { |
There was a problem hiding this comment.
NitPick: let's break it down to multi line
| import { ButtonType } from "../constants"; | ||
| import { LOG_TAG } from "../constants"; | ||
|
|
||
| export const useMiniPlayerMediaStatusListenerHook = ({ |
There was a problem hiding this comment.
Let's use consistent naming starting from Component folder name to hooks use MiniCastBar
say,
- For component folder name
VizbeeMiniCastBar - For functional component name
VizbeeMiniCastBar.js - For hooks
useMiniCastBarMediaStatusListenerHook.js
| case VizbeePlayerState.Stopped: | ||
| case VizbeePlayerState.Stopped_On_Disconnect: | ||
| case VizbeePlayerState.Ended: | ||
| // Receiving started event with empty title and subtitle so remo |
There was a problem hiding this comment.
NitPick: finish the comment
…ght greater then 64.
| * @returns {string} title to display | ||
| */ | ||
| export const getTitleFromVideoInfo = ({title, castingTo}) => { | ||
| if (title && title?.length) { |
There was a problem hiding this comment.
?. is optional chaining operator is already used here. So no need to check title. Please change title && title?.length to title?.length
| * @returns {string} subtitle to display | ||
| */ | ||
| export const getSubTitleFromVideoInfo = ({title, subTitle, castingTo}) => { | ||
| if (subTitle && subTitle?.length) { |
There was a problem hiding this comment.
Change if (subTitle && subTitle?.length) to if (subTitle?.length) and if (title && title?.length) to if(title?.length)
| * | ||
| * @returns {Object} default color object | ||
| */ | ||
| export function getApperanceColor(isDark) { |
There was a problem hiding this comment.
is this typo? getApperanceColor --> getAppearanceColor
| console.debug( | ||
| `${LOG_TAG} Media status current state :: ${newMediaStatus.playerState}` | ||
| ); | ||
| if (newMediaStatus?.guid && newMediaStatus?.guid?.length != 0) { |
There was a problem hiding this comment.
Same here: Change if (newMediaStatus?.guid && newMediaStatus?.guid?.length != 0) to if(newMediaStatus?.guid?.length != 0)
| }) => { | ||
| const isDark = Appearance.getColorScheme() === "dark"; | ||
|
|
||
| const defaultApperance = getApperanceColor(isDark); |
There was a problem hiding this comment.
is this typo? defaultApperance --> defaultAppearanceandgetApperanceColor-->getAppearanceColor`
| @@ -0,0 +1,41 @@ | |||
| import { VizbeeManager } from "../../../../"; | |||
There was a problem hiding this comment.
Is this a react custom hook? We are not using any react APIs like useState or useEffect or anything.
There was a problem hiding this comment.
There was no need of it in this hook
Added Mini player component using existing Native api's.