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

fix(h5 video): create video context #7605

Merged
merged 7 commits into from
Oct 17, 2020

Conversation

helsonxiao
Copy link
Contributor

@helsonxiao helsonxiao commented Sep 14, 2020

这个 PR 做了什么? (简要描述所做更改)

  • 执行这些步骤以后 lock 文件发生了变化,补充 commit
  • 修复在 H5 中完全无法使用的 createVideoContext API
  • 增加测试
  • 支持其它 VideoContext API ?
  • 完善兼容性

这个 PR 是什么类型? (至少选择一个)

  • 错误修复(Bugfix) issue id #
  • 新功能(Feature)
  • 代码重构(Refactor)
  • TypeScript 类型定义修改(Typings)
  • 文档修改(Docs)
  • 代码风格更新(Code style update)
  • 其他,请描述(Other, please describe):

这个 PR 满足以下需求:

  • 提交到 master 分支
  • Commit 信息遵循 Angular Style Commit Message Conventions
  • 所有测试用例已经通过
  • 代码遵循相关包中的 .eslintrc, .tslintrc, .stylelintrc 所规定的规范
  • 在本地测试可用,不会影响到其它功能

这个 PR 涉及以下平台:

  • 微信小程序
  • 支付宝小程序
  • 百度小程序
  • 头条小程序
  • QQ 轻应用
  • 快应用平台(QuickApp)
  • Web 平台(H5)
  • 移动端(React-Native)

其它需要 Reviewer 或社区知晓的内容:

@helsonxiao helsonxiao force-pushed the helson/fix-h5-createVideoContext branch from 0322091 to 098249d Compare September 14, 2020 02:52
@helsonxiao helsonxiao changed the title fix(taro-h5): create video context fix(h5 video): create video context Sep 14, 2020
@helsonxiao helsonxiao force-pushed the helson/fix-h5-createVideoContext branch from 098249d to 4fad6e0 Compare September 14, 2020 05:08
@helsonxiao helsonxiao marked this pull request as ready for review September 14, 2020 05:49
@Chen-jj Chen-jj added the V-3 Version - 3.x label Sep 15, 2020
@helsonxiao helsonxiao force-pushed the helson/fix-h5-createVideoContext branch from 1e8b780 to 1611dbd Compare September 20, 2020 14:24
@helsonxiao
Copy link
Contributor Author

hi @ZakaryCode , 我想请问一下 yarn bootstrap 修改了好多 yarn.lock 文件,我需要提交上来吗?因为我看到你 merge 之后把它们都取消了,我不确定要不要提交上来。以及,这个 PR 还有哪些地方要改的吗?我会再补充一些测试。

@ZakaryCode
Copy link
Contributor

hi @ZakaryCode , 我想请问一下 yarn bootstrap 修改了好多 yarn.lock 文件,我需要提交上来吗?因为我看到你 merge 之后把它们都取消了,我不确定要不要提交上来。以及,这个 PR 还有哪些地方要改的吗?我会再补充一些测试。

那是因为合并的时候冲突了,如果没有新增依赖,一般不需要提交 lock 文件

@helsonxiao
Copy link
Contributor Author

hi @ZakaryCode , 我想请问一下 yarn bootstrap 修改了好多 yarn.lock 文件,我需要提交上来吗?因为我看到你 merge 之后把它们都取消了,我不确定要不要提交上来。以及,这个 PR 还有哪些地方要改的吗?我会再补充一些测试。

那是因为合并的时候冲突了,如果没有新增依赖,一般不需要提交 lock 文件

那我重新基于 next 分支 rebase 一下吧,lock 我就不提交了,测试好像挂了我再看看

@helsonxiao helsonxiao force-pushed the helson/fix-h5-createVideoContext branch from 3dcf67e to 7be9696 Compare October 16, 2020 09:03
@helsonxiao helsonxiao force-pushed the helson/fix-h5-createVideoContext branch from 7be9696 to 85871fe Compare October 16, 2020 09:08
@helsonxiao
Copy link
Contributor Author

helsonxiao commented Oct 16, 2020

@ZakaryCode 我增加了一些兼容性的处理,不过奇怪的是自从 merge 最新分支之后,测试就跑不过了(我现在修改了实现方法,现在测试能过,但是可能不那么优雅),之前都可以的。我不知道你们都有什么方便的测试方法,现在我是把 build 出来的 dist / dist-h5 都覆盖我自己的项目。我贴一下测试用的代码吧。

import React, { Component } from 'react'
import Taro from '@tarojs/taro'
import { Swiper, SwiperItem, Video } from '@tarojs/components'
import './index.scss'

const videos = [
  {
    id: 'video-1',
    src: 'http://storage.jd.com/cjj-pub-images/bear.mp4',
  },
  {
    id: 'video-2',
    src: 'http://storage.jd.com/cjj-pub-images/bear.mp4',
  },
  {
    id: 'video-3',
    src: 'http://storage.jd.com/cjj-pub-images/bear.mp4',
  },
]

const videoRefMap = {};

export default class Index extends Component {

  componentDidMount () {
    videos.forEach((v, index) => {
      videoRefMap[index] = Taro.createVideoContext(v.id);
    })
  }

  render () {
    return (
      <Swiper
        vertical
        circular={false}
        onChange={e => {
          Object.keys(videoRefMap).forEach(k => {
            if (e.detail.current === k) return;
            videoRefMap[k].stop();
          })
          videoRefMap[e.detail.current].play();
        }}
      >
        {videos.map((v, index) => (
          <SwiperItem key={index}>
            <Video id={v.id} src={v.src} />
          </SwiperItem>
        ))}
      </Swiper>
    )
  }
}
// h5
taro-swiper-core {
  height: 100vh !important;
}

.swiper-slide {
  height: 100% !important;
}

.taro-video {
  height: 100% !important;
}

// weapp
swiper {
  height: 100vh;
}

video {
  width: 100%;
  height: 100%;
}
cd packages/taro-components
npx karmatic --files './__tests__/video.spec.js' --coverage false

@helsonxiao
Copy link
Contributor Author

helsonxiao commented Oct 16, 2020

有几点需要在 pr 里阐述一下:

  1. play / pause 等方法没有自动生成注释的原因是因为它们是保留关键字,但是为了和小程序的 API 拉齐,只能叫这几个名字
  2. testing 参数的作用是为了在测试中仅更新状态,不调用 requestFullscreen 这个方法,不然跑不通,标签的方法都是 read only 也没法 mock 掉?我会再尝试一下写个测试专用的 stencil component,把浏览器的 API mock 掉。
  3. 修复了 test 环境中疑似 ref 未转发的 bug

@helsonxiao helsonxiao changed the title fix(h5 video): create video context WIP - fix(h5 video): create video context Oct 17, 2020
@ZakaryCode
Copy link
Contributor

ZakaryCode commented Oct 17, 2020

有几点需要在 pr 里阐述一下:

  1. play / pause 等方法没有自动生成注释的原因是因为它们是保留关键字,但是为了和小程序的 API 拉齐,只能叫这几个名字
  2. testing 参数的作用是为了在测试中仅更新状态,不调用 requestFullscreen 这个方法,不然跑不通,标签的方法都是 read only 也没法 mock 掉?我会再尝试一下写个测试专用的 stencil component,把浏览器的 API mock 掉。
  3. 修复了 test 环境中疑似 ref 未转发的 bug

前两个问题已通过其方案修复,第三个问题暂无必要,已回退相关内容

@helsonxiao
Copy link
Contributor Author

有几点需要在 pr 里阐述一下:

  1. play / pause 等方法没有自动生成注释的原因是因为它们是保留关键字,但是为了和小程序的 API 拉齐,只能叫这几个名字
  2. testing 参数的作用是为了在测试中仅更新状态,不调用 requestFullscreen 这个方法,不然跑不通,标签的方法都是 read only 也没法 mock 掉?我会再尝试一下写个测试专用的 stencil component,把浏览器的 API mock 掉。
  3. 修复了 test 环境中疑似 ref 未转发的 bug

前两个问题已通过其方案修复,第三个问题暂无必要,已回退相关内容

👌,不过好像混入了没必要的东西,还有我的兼容性函数被误删了。

@ZakaryCode
Copy link
Contributor

ZakaryCode commented Oct 17, 2020

有几点需要在 pr 里阐述一下:

  1. play / pause 等方法没有自动生成注释的原因是因为它们是保留关键字,但是为了和小程序的 API 拉齐,只能叫这几个名字
  2. testing 参数的作用是为了在测试中仅更新状态,不调用 requestFullscreen 这个方法,不然跑不通,标签的方法都是 read only 也没法 mock 掉?我会再尝试一下写个测试专用的 stencil component,把浏览器的 API mock 掉。
  3. 修复了 test 环境中疑似 ref 未转发的 bug

前两个问题已通过其方案修复,第三个问题暂无必要,已回退相关内容

👌,不过好像混入了没必要的东西,还有我的兼容性函数被误删了。

OK,兼容性函数补回来了咯,然后是删掉了些冗余并且优化了 Video 的部分特性

@ZakaryCode
Copy link
Contributor

感谢贡献,准备 Merge 咯

@helsonxiao helsonxiao changed the title WIP - fix(h5 video): create video context fix(h5 video): create video context Oct 17, 2020
Copy link
Contributor

@ZakaryCode ZakaryCode left a comment

Choose a reason for hiding this comment

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

merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
V-3 Version - 3.x
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants