-
-
Notifications
You must be signed in to change notification settings - Fork 3
feature: drag the progress volume bar #22
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
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 908c8c7:
|
Codecov Report
@@ Coverage Diff @@
## master #22 +/- ##
==========================================
- Coverage 26.33% 26.29% -0.05%
==========================================
Files 17 18 +1
Lines 1257 1316 +59
Branches 46 48 +2
==========================================
+ Hits 331 346 +15
- Misses 925 969 +44
Partials 1 1
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
SevenOutman
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @raotaohub, thanks for refreshing this edit to be up-to-date with the latest codebase! The changes look good to me, but there're still some minor changes I would like you to make.
src/components/volume.tsx
Outdated
| return ( | ||
| <div | ||
| className={clsx("aplayer-volume-wrap", { | ||
| "aplayer-volume-bar-wrap-active": isDragging, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
aplayer-volume-bar-wrap-active class does not seem necessary here. It's already added on the .aplayer-volume-bar-wrap element.
src/components/volume.tsx
Outdated
| volume: number; | ||
| muted: boolean; | ||
| onToggleMuted: () => void; | ||
| onChangeVolume: (vaolume: number) => void; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A typo.
| onChangeVolume: (vaolume: number) => void; | |
| onChangeVolume: (volume: number) => void; |
src/utils/computePercentage.test.ts
Outdated
| test("Return 0 if volumeBarRef.current is undefined", () => { | ||
| expect( | ||
| computePercentageOfY(new MouseEvent("mousedown"), { | ||
| current: null, | ||
| }) | ||
| ).toBe(0); | ||
| expect( | ||
| computePercentageOfY(new MouseEvent("mousedown"), { | ||
| current: null, | ||
| }) | ||
| ).toBe(0); | ||
| expect( | ||
| computePercentageOfY(new MouseEvent("mousedown"), { | ||
| current: null, | ||
| }) | ||
| ).toBe(0); | ||
| }); | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As there's more than one functions being test here, please group their tests accordingly using the describe API, so that the tests look clearer.
|
hi @SevenOutman . |
SevenOutman
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM now, thanks!
|
🎉 This PR is included in version 1.4.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
rewritten according to
components/progress.tsx。