Skip to content
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

feat: replace onFullScreenChange with onEvent #175

Merged
merged 1 commit into from
May 26, 2021
Merged

Conversation

ambar
Copy link
Collaborator

@ambar ambar commented May 26, 2021

添加 onEvent 作为 MessageContext 通讯的替代方式,MessageContext 有两个缺陷:

  • 它依赖 didmount/useEffect,如果触发的事件比较早就会监听不到
  • 使用比较麻烦,需要注销监听,并且依赖额外的包(griffith-message),这个 PR 同时把 EVENTS 对象导出了

现在的替代使用方式是:

import Player, {EVENTS} from 'griffith'

render(
  <Player
    onEvent={type => {
      if (type === EVENTS.PLAYER.REQUEST_PLAY) {
        ...
      }
    }}
  />
)

新旧使用方式在 example/src/MP4Page.jsx 中都有体现。

@codecov-commenter
Copy link

codecov-commenter commented May 26, 2021

Codecov Report

Merging #175 (c5caf55) into master (2d8d444) will increase coverage by 0.05%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #175      +/-   ##
==========================================
+ Coverage   48.67%   48.72%   +0.05%     
==========================================
  Files         128      128              
  Lines        1847     1845       -2     
==========================================
  Hits          899      899              
+ Misses        948      946       -2     
Impacted Files Coverage Δ
packages/griffith/src/components/Player/Player.js 0.00% <0.00%> (ø)
.../src/components/PlayerContainer/PlayerContainer.js 0.00% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2d8d444...c5caf55. Read the comment docs.

@@ -45,7 +46,7 @@ const PlayerContainer = ({
<InternalContext.Consumer>
{({emitEvent, subscribeAction}) => (
<VideoSourceProvider
onEvent={emitEvent}
onEvent={sequence(emitEvent, onEvent || noop)}
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

应该 hook 化,把这里的所有 Consumer 都删掉(在各自组件内使用 useContext),props 就能直接传递下去不需要 sequence

@ambar ambar merged commit 9567337 into zhihu:master May 26, 2021
@ambar ambar deleted the onevent branch September 13, 2021 05:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants