feat(components/atom/videoPlayer): add factory hook #2524
Conversation
andresin87
requested review from
kikoruiz,
sui-bot,
arnau-rius and
andresz1
as code owners
andresin87
requested review from
oegea
and removed request for
kikoruiz,
andresz1,
arnau-rius and
sui-bot
|
Despite of reducing the relative coverage, it is better not increasing (reduces) in production the number of code lines, functions and statements |
|
Adds polymorphism and fwdRef |
| const useVideoPlayer = ({ | ||
| as: As = 'div', | ||
| children = 'Not supported media type', | ||
| className, | ||
| src = '', | ||
| ...props | ||
| } = {}) => { | ||
| if (matcher([YOUTUBE.VIDEO_TYPE, YOUTUBE.SHORT_URL], src)) { | ||
| return [ | ||
| YouTubePlayer, | ||
| { | ||
| className: [BASE_CLASS, `${BASE_CLASS}-youtubePlayer`].join(' '), | ||
| src, | ||
| ...props | ||
| } | ||
| ] | ||
| } else if (matcher([VIMEO.VIDEO_TYPE], src)) { | ||
| return [ | ||
| VimeoPlayer, | ||
| { | ||
| className: [BASE_CLASS, `${BASE_CLASS}-vimeoPlayer`].join(' '), | ||
| src, | ||
| ...props | ||
| } | ||
| ] | ||
| } | ||
| return ['h1', {className: BASE_CLASS, children, ...props}] | ||
| } |
There was a problem hiding this comment.
Thank you for your proposal.
I'm wondering if this is an agreement of how to create these kind of structures in Sui.
Probably I'm missing the benefit or architectural pattern behind this change, but IMO this is making the same effect than the already existing code, but in a less obvious way, delegating some representation responsibilities to the useVideoPlayer hook.
All of this is still entirely my opinion, but without knowing the benefits of applying this alternative approach, it's more difficult in my opinion to read the code and follow the logic flow. For example is less obvious were the vimeoPlayer style class is used. In the already existing approach, the Vimeo player component self-contains both all the needed logic and css class references, and the switch mechanism has a very specific and limited responsibility.
Maybe we can have a discussion about this when you have a moment for us!
There was a problem hiding this comment.
reduces the VDOM tree reconciliation. Take a look on react-dev-tools, there is non Switcher component
| export default function AtomVideoPlayer({src = ''}) { | ||
| import useVideoPlayer from './hooks/useVideoPlayer.js' | ||
|
|
||
| const AtomVideoPlayer = forwardRef((props, forwardedRef) => { |
There was a problem hiding this comment.
Thank you, we totally forgot about forwarding ref.
Atom/VideoPlayer
❓ AskTASK:
Description, Motivation and Context
Types of changes
Screenshots - Animations