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.

Sign up for GitHub

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

Jump to bottom

Move libraries used for interactions to webpack #10388

Merged
merged 14 commits into from
Aug 24, 2020
Merged

Move libraries used for interactions to webpack #10388

merged 14 commits into from
Aug 24, 2020

Conversation

nishantwrp
Copy link
Contributor

Overview

  1. This PR fixes or fixes part of #[fill_in_number_here].
  2. This PR does the following: Move libraries used for interactions using webpack

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 Aug 19, 2020

Hi, @nishantwrp, this pull request does not have a "CHANGELOG: ..." label as mentioned in the PR checkbox list. Please add this label. PRs without this label will not be merged. If you are unsure of which label to add, please ask the reviewers for guidance. Thanks!

@oppiabot
Copy link

oppiabot bot commented Aug 19, 2020

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

vojtechjelinek
vojtechjelinek suggested changes Aug 20, 2020
View reviewed changes
Copy link
Contributor

@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.

Thanks! A two questions.

core/templates/pages/exploration-editor-page/exploration-editor-page.module.ts
Comment on lines +24 to +28
import 'third-party-imports/guppy.import';
import 'third-party-imports/midi-js.import';
import 'third-party-imports/ng-audio.import';
import 'third-party-imports/ng-joy-ride.import';
import 'third-party-imports/skulpt.import';
Copy link
Contributor

Choose a reason for hiding this comment

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

Could any of these newly added libs be only imported at places where they are actually needed? Like in some directive where they are used.

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 the issue with that is these imports look like this

window.Guppy = ...

and requiring this multiple times loads the library script many times. Plus anyways they need to be loaded in exploration editor and player page. So, added these here to load the script only once.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, that makes sense.

core/templates/pages/review-test-page/review-test-page.mainpage.html
@@ -24,6 +24,5 @@
</oppia-root>
@load('pages/footer_js_libs.html')
<script src="/third_party/static/MathJax-2.7.5/MathJax.js?config=default"></script>
@loadExtensions('interactions/codemirror.html')
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is it no longer needed here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

its added in the respective directive.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok.

vojtechjelinek
vojtechjelinek approved these changes Aug 21, 2020
View reviewed changes
Copy link
Contributor

@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.

LGTM!

@vojtechjelinek vojtechjelinek removed their assignment Aug 21, 2020
ankita240796
ankita240796 approved these changes Aug 21, 2020
View reviewed changes
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 Aug 21, 2020
@oppiabot
Copy link

oppiabot bot commented Aug 21, 2020

Hi @nishantwrp. Due to recent changes in the "develop" branch, this PR now has a merge conflict. Please follow this link if you need help resolving the conflict, so that the PR can be merged. Thanks!

DubeySandeep
DubeySandeep reviewed Aug 21, 2020
View reviewed changes
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.

LGTM for the core/templates/pages/contributor-dashboard-page/question-opportunities/question-opportunities.directive.ts file changes!

@DubeySandeep DubeySandeep removed their assignment Aug 21, 2020
kevintab95
kevintab95 approved these changes Aug 21, 2020
View reviewed changes
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.

Codeowner files LGTM, thanks @nishantwrp!

@kevintab95 kevintab95 removed their assignment Aug 21, 2020
@oppiabot
Copy link

oppiabot bot commented Aug 21, 2020

Hi @nishantwrp. Due to recent changes in the "develop" branch, this PR now has a merge conflict. Please follow this link if you need help resolving the conflict, so that the PR can be merged. Thanks!

Showtim3
Showtim3 approved these changes Aug 22, 2020
View reviewed changes
Copy link
Contributor

@Showtim3 Showtim3 left a comment

Choose a reason for hiding this comment

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

LGTM

bansalnitish
bansalnitish approved these changes Aug 22, 2020
View reviewed changes
Copy link
Contributor

@bansalnitish bansalnitish 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 codeowner file.

@bansalnitish bansalnitish removed their assignment Aug 22, 2020
@Showtim3 Showtim3 removed their assignment Aug 22, 2020
@nishantwrp
Copy link
Contributor Author

nishantwrp commented Aug 23, 2020 •
edited
Loading

@U8NWXD @kevinlee12 There are two Travis CI checks in this pr. Any ideas on how to fix this?

@nishantwrp nishantwrp changed the title Move libraries used for interactions using webpack Move libraries used for interactions to webpack Aug 23, 2020
@kevinlee12
Copy link
Contributor

@nishantwrp Travis does that sometimes, as long as the PR isn't blocked for merge (assuming all reviewers ok), then it's fine.

@aks681 aks681 assigned nishantwrp and unassigned aks681 Aug 24, 2020
@nishantwrp nishantwrp merged commit 65a1e80 into oppia:develop Aug 24, 2020
@nishantwrp nishantwrp deleted the interaction-libs-webpack-final branch August 24, 2020 23:10
@nishantwrp nishantwrp mentioned this pull request Aug 26, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Showtim3 Showtim3 Showtim3 approved these changes

@bansalnitish bansalnitish bansalnitish approved these changes

@aks681 aks681 aks681 approved these changes

@DubeySandeep DubeySandeep DubeySandeep approved these changes

@ankita240796 ankita240796 ankita240796 approved these changes

@kevintab95 kevintab95 kevintab95 approved these changes

@vojtechjelinek vojtechjelinek vojtechjelinek approved these changes

Assignees

@nishantwrp nishantwrp

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@nishantwrp @kevinlee12 @vojtechjelinek @kevintab95 @ankita240796 @DubeySandeep @aks681 @bansalnitish @Showtim3