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

Milestone 1.3.2: Import WaveSurfer using yarn and add typedefs for it #9279

Merged
merged 8 commits into from May 11, 2020
Merged

Milestone 1.3.2: Import WaveSurfer using yarn and add typedefs for it #9279

merged 8 commits into from May 11, 2020

Conversation

nishantwrp
Copy link
Contributor

@nishantwrp nishantwrp commented May 9, 2020

Overview

1. This PR fixes or fixes part of #[fill_in_number_here]. #6351
2. This PR does the following: Import WaveSurfer using yarn

Essential Checklist

  • The PR title starts with "Fix #bugnum: ", followed by a short, clear summary of the changes. (If this PR fixes part of an issue, prefix the title with "Fix part of #bugnum: ...".)
  • The linter/Karma presubmit checks have passed locally on your machine.
  • "Allow edits from maintainers" is checked. (See here for instructions on how to enable it.)
    • This lets reviewers restart your CircleCI tests for you.
  • The PR is made from a branch that's not called "develop".

PR Pointers

  • Oppiabot will notify you when you don't add a PR_CHANGELOG label. If you are unable to do so, please @-mention a code owner (who will be in the Reviewers list), or ask on Gitter.
  • For what code owners will expect, see the Code Owner's wiki page.
  • Make sure your PR follows conventions in the style guide, otherwise this will lead to review delays
  • Never force push. If you do, your PR will be closed.

@oppiabot
Copy link

oppiabot bot commented May 9, 2020

Assigning @vojtechjelinek for the first-pass review of this pull request. Thanks!

Copy link
Member

@vojtechjelinek vojtechjelinek left a comment

Choose a reason for hiding this comment

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

Nice! Thanks, LGTM!

@vojtechjelinek
Copy link
Member

@kevinlee12 @kevintab95 @DubeySandeep @aks681 @marianazangrossi PTAL

@nishantwrp nishantwrp marked this pull request as draft May 9, 2020 13:28
@nishantwrp nishantwrp marked this pull request as ready for review May 9, 2020 13:38
@nishantwrp nishantwrp changed the title Import WaveSurfer using yarn Milestone 1.2.2: Import WaveSurfer using yarn May 9, 2020
@nishantwrp nishantwrp changed the title Milestone 1.2.2: Import WaveSurfer using yarn Milestone 1.2.2: Import WaveSurfer using yarn and add typedefs for it May 9, 2020
@nishantwrp nishantwrp changed the title Milestone 1.2.2: Import WaveSurfer using yarn and add typedefs for it Milestone 1.3.2: Import WaveSurfer using yarn and add typedefs for it May 9, 2020
aks681
aks681 approved these changes May 9, 2020
Copy link
Member

@aks681 aks681 left a comment

Choose a reason for hiding this comment

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

Lgtm for the one codeowner file.

@aks681 aks681 removed their assignment May 9, 2020
Copy link
Contributor

@kevinlee12 kevinlee12 left a comment

Choose a reason for hiding this comment

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

lgtm!

@kevinlee12 kevinlee12 removed their assignment May 9, 2020
Copy link
Member

@kevintab95 kevintab95 left a comment

Choose a reason for hiding this comment

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

LGTM for code-owner file. Thanks @nishantwrp!

@kevintab95 kevintab95 removed their assignment May 9, 2020
Copy link
Contributor

@ankita240796 ankita240796 left a comment

Choose a reason for hiding this comment

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

LGTM from code-owner's perspective, Thanks @nishantwrp!

@ankita240796 ankita240796 removed their assignment May 9, 2020
Copy link
Member

@DubeySandeep DubeySandeep left a comment

Choose a reason for hiding this comment

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

@nishantwrp, Thanks for the PR, I have left a few questions for the changes below, PTAL!

@@ -48,7 +48,8 @@ require('services/context.service.ts');
require('services/editability.service.ts');
require('services/id-generation.service.ts');
require('services/user.service.ts');
const WaveSurfer = require('third-party-imports/wave-surfer.import.ts');

import WaveSurfer from 'wavesurfer.js';
Copy link
Member

Choose a reason for hiding this comment

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

Do wee need parenthesis around WaveSurfer?
Also, can you please share the image of the audio waves in the translation tab after this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No we don't need parenthesis.

Here is an image
Screenshot_20200510_032047

package.json Outdated
@@ -112,6 +112,7 @@
"gifshot": "^0.4.5",
"lodash": "^4.17.14",
"mathjs": "^6.6.4",
"moment": "^2.24.0"
"moment": "^2.24.0",
"wavesurfer.js": "^3.3.3"
Copy link
Member

Choose a reason for hiding this comment

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

Can you please test this feature thoroughly (as you update the version from 2.2.1 to 3.3.3)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The translation was being played correctly. What other things do I need to test?

Copy link
Member

Choose a reason for hiding this comment

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

Slider bar, rendering, check the console, wave for small audio wave for big audio (intime or size). etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually I’m not sure about the expected behaviour. Can you do that please?

Copy link
Member

Choose a reason for hiding this comment

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

Btw, can you set it to "3.3.3" without ^ so that it doesn't get updated automatically and the coherence with the type definitions is kept.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@@ -0,0 +1,73 @@
// Code - https://raw.githubusercontent.com/katspaugh/wavesurfer.js/master/src/wavesurfer.js
Copy link
Member

Choose a reason for hiding this comment

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

Wanted to check whether we need to update this whenever we update the wave surfer version?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, will add a lint check later on for this too.

Copy link
Member

Choose a reason for hiding this comment

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

Do we have any (github) issue for that? Also, how would we catch this till we have this check?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, actually the lint check will not check that the code link is correct. It will be there so that the dev is aware that the type defs were for a previous version and they may have changed now.

Copy link
Member

Choose a reason for hiding this comment

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

Still, my question persists i.e, until we don't have that "warning through lint checks" how would we catch this during any update?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess we can do that manually, as I don't think type defs would be updated so frequently.

Copy link
Member

Choose a reason for hiding this comment

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

@DubeySandeep Since I'm mostly responsible for library upgrades I can make sure that the type definitions are correct until we have the lint check.

@@ -0,0 +1,73 @@
// Code - https://raw.githubusercontent.com/katspaugh/wavesurfer.js/master/src/wavesurfer.js
Copy link
Member

Choose a reason for hiding this comment

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

Also, why are we using the master branch file here? Should we use this file from v3.3.3?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, yeah correct. Done

Copy link
Member

Choose a reason for hiding this comment

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

Have you tested the type.defs with the file in v3.3.3 (as it can be different from master branch)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup! Did that just now.

@nishantwrp
Copy link
Contributor Author

@DubeySandeep Addressed your comments. PTAL!

@vojtechjelinek
Copy link
Member

@DubeySandeep @marianazangrossi PTAL

@codecov
Copy link

codecov bot commented May 10, 2020

Codecov Report

Merging #9279 into develop will decrease coverage by 0.01%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           develop    #9279      +/-   ##
===========================================
- Coverage    53.54%   53.54%   -0.01%     
===========================================
  Files          875      874       -1     
  Lines        36278    36284       +6     
  Branches      4295     4295              
===========================================
+ Hits         19424    19425       +1     
- Misses       15908    15913       +5     
  Partials       946      946              
Flag Coverage Δ
#frontend 53.54% <100.00%> (-0.01%) ⬇️
Impacted Files Coverage Δ
...translation-bar/audio-translation-bar.directive.ts 6.21% <100.00%> (ø)
.../topic-editor-page/topic-editor-page.controller.ts 57.14% <0.00%> (-2.86%) ⬇️
...entity-creation-services/topic-creation.service.ts 12.50% <0.00%> (-1.29%) ⬇️
...ions/interactions/interactionsQuestionsRequires.ts 100.00% <0.00%> (ø)

Copy link
Contributor

@marianadasilvadev marianadasilvadev left a comment

Choose a reason for hiding this comment

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

LGTM as codeowner!

Copy link
Member

@DubeySandeep DubeySandeep left a comment

Choose a reason for hiding this comment

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

@nishantwrp, Just a last few minor comments, PTAL!

@@ -112,6 +112,7 @@
"gifshot": "^0.4.5",
"lodash": "^4.17.14",
"mathjs": "^6.6.4",
"moment": "^2.24.0"
"moment": "^2.24.0",
"wavesurfer.js": "3.3.3"
Copy link
Member

Choose a reason for hiding this comment

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

Can you please create an issue to add '^` here once we have the warning functionality? [This will help us to track this task]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done #9287

@@ -0,0 +1,81 @@
// Code (Release 3.3) - https://github.com/katspaugh/wavesurfer.js/blob/4421b44e205c76359407ae995dcb8b8453f903a1/src/wavesurfer.js
Copy link
Member

Choose a reason for hiding this comment

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

Release 3.3.3*

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

@@ -0,0 +1,81 @@
// Code (Release 3.3) - https://github.com/katspaugh/wavesurfer.js/blob/4421b44e205c76359407ae995dcb8b8453f903a1/src/wavesurfer.js
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

@DubeySandeep DubeySandeep merged commit bb4edae into oppia:develop May 11, 2020
17 of 18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment