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(the-video-controller): fix some UI bugs #192

Merged
merged 19 commits into from
Sep 20, 2018

Conversation

YvonTre
Copy link
Contributor

@YvonTre YvonTre commented Sep 19, 2018

  • Increase TheVideoController's test coverage to 84.62%.
  • Fix Unfocused window's problems:
    • Mouseup will trigger PlayButton.
    • Dragging will trigger playback.
    • Cursor will suddenly disappear.
  • Fix Mac titlebar minimize button toggle close.

@codecov
Copy link

codecov bot commented Sep 19, 2018

Codecov Report

Merging #192 into develop will increase coverage by 1.18%.
The diff coverage is 57.57%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #192      +/-   ##
===========================================
+ Coverage    62.01%   63.19%   +1.18%     
===========================================
  Files           48       48              
  Lines         2364     2372       +8     
  Branches       301      300       -1     
===========================================
+ Hits          1466     1499      +33     
+ Misses         778      757      -21     
+ Partials       120      116       -4
Impacted Files Coverage Δ
src/renderer/components/Titlebar.vue 32.6% <ø> (ø) ⬆️
...rer/components/PlayingView/ThePreviewThumbnail.vue 39.65% <ø> (+10.34%) ⬆️
.../renderer/components/PlayingView/VolumeControl.vue 47.05% <ø> (ø) ⬆️
...enderer/components/PlayingView/SubtitleControl.vue 52.17% <ø> (-1.45%) ⬇️
...enderer/components/PlayingView/TimeProgressBar.vue 60% <100%> (+3.2%) ⬆️
...rc/renderer/components/PlayingView/VideoCanvas.vue 64.77% <100%> (+3.62%) ⬆️
src/renderer/helpers/timerManager.js 100% <100%> (ø) ⬆️
...erer/components/PlayingView/TheVideoController.vue 80.51% <60%> (+12.98%) ⬆️
src/renderer/main.js 40% <9.09%> (-3.81%) ⬇️
...c/renderer/components/PlayingView/BaseSubtitle.vue 68.73% <0%> (-2.43%) ⬇️
... and 5 more

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 64cdc0f...2789309. Read the comment docs.

import Titlebar from '../Titlebar.vue';
import PlayButton from './PlayButton.vue';
import VolumeControl from './VolumeControl';
import AdvanceControl from './AdvanceControl';
import SubtitleControl from './SubtitleControl';
import TheTimeCodes from './TheTimeCodes';
import TimeProgressBar from './TimeProgressBar.vue';
import TimeProgressBar from './TimeProgressBar';
Copy link
Collaborator

Choose a reason for hiding this comment

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

这里为什么会去掉 .vue 呢?
貌似用 vscode 的话不带 .vue 无法识别

Copy link
Contributor Author

Choose a reason for hiding this comment

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

我试了下没有.vue我这边没问题,但是这个链接说把所有的拓展名加上去会更好一点。
要不要把所有的拓展名(.js, .vue)都加上去呢。

Copy link
Collaborator

Choose a reason for hiding this comment

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

我个人习惯是不带,这样如果有 js 迁移到 ts 这种操作只要改下文件名就好,不用改代码。另一个场景是类似 React Native 或者 Meteor 里的代码可能会跨多个平台,这样写 import * from 'index' 可以在不同平台下分别映射到 index.ios.js, index.android.js, index.server.js
不过我们这个项目不太会有这样的情况,而且 vue 也是个比较特殊的格式,里面包括了多种类型的内容,可以都带上扩展名。
反正约定统一就好~

Copy link
Contributor Author

Choose a reason for hiding this comment

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

那我先把这个文件内部的import全部加上拓展名了。

this.timerManager.updateTimer('mouseStopMoving', this.mousestopDelay, false);
this.mouseStopMoving = false;
}
this.mouseStopMoving = currentPosition === lastPosition;
Copy link
Collaborator

Choose a reason for hiding this comment

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

这里用深比较会不会合理点?好像没有看到单独设置 position 的地方,浅比较 position 相同的话 currentEventInfo.get('mousemove') 和 lastEventInfo.get('mousemove') 也是相同的吧

Copy link
Contributor Author

Choose a reason for hiding this comment

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

对的,用深比较会更合理。那我再加上一句/* eslint-disable eqeqeq */么,好像规则不太想用==

Copy link
Collaborator

Choose a reason for hiding this comment

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

== 不是深比较啊 - -||
{a:1} == {a:1} 也是 false 的。深比较是指 ({a:1}).a === ({a:1}).a 这种

Copy link
Contributor Author

Choose a reason for hiding this comment

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

好的

src/renderer/components/PlayingView/VideoCanvas.vue Outdated Show resolved Hide resolved
@huzhenghui37
Copy link
Collaborator

我这里没什么问题,OK的。

@YvonTre
Copy link
Contributor Author

YvonTre commented Sep 20, 2018

@YvonTre YvonTre closed this Sep 20, 2018
@YvonTre YvonTre reopened this Sep 20, 2018
@ipy ipy merged commit 26d56dd into chiflix:develop Sep 20, 2018
@YvonTre YvonTre deleted the UI-mechanism-rework branch September 25, 2018 05:16
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