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

Move scripts imports in header to webpack #10141

Merged
merged 12 commits into from Aug 8, 2020
Merged

Move scripts imports in header to webpack #10141

merged 12 commits into from Aug 8, 2020

Conversation

nishantwrp
Copy link
Contributor

@nishantwrp nishantwrp commented Aug 3, 2020

Overview

  1. This PR fixes or fixes part of #[fill_in_number_here].
  2. This PR does the following: Move scripts imports in header to 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 3, 2020

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

@nishantwrp nishantwrp requested a review from seanlip as a code owner Aug 3, 2020
Copy link
Member

@vojtechjelinek vojtechjelinek left a comment

Thanks! Few comments.

core/templates/third-party-imports/jquery.import.ts Outdated Show resolved Hide resolved
// This scrolls the page to the anchor clicked on in the quick links menu.
$('a[href*=\\#]:not([href=\\#])').click(function() {
if (location.pathname.replace(/^\//, '') == this.pathname.replace(/^\//, '')) {
var target = $('[name=' + this.hash.slice(1) +']');
if (target.length) {
$('html,body').animate({
scrollTop: target.offset().top
}, 1000);
return false;
}
}
});
Copy link
Member

@vojtechjelinek vojtechjelinek Aug 4, 2020

Choose a reason for hiding this comment

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

Why is this moved, also it is possible to replace this by Angular? We want to limit the direct usage of jQuery as much as possible.

Copy link
Contributor Author

@nishantwrp nishantwrp Aug 4, 2020

Choose a reason for hiding this comment

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

Why is this moved

It uses jQuery and jQuery is now loaded in the webpack bundle, so, it needs to be after the footer_libs import.

is it possible to replace this by Angular?

This page currently has no controller. It's just a mainpage.html.

Also, I think the smooth scroll doesn't work. You can see it here -> https://www.oppia.org/terms

Copy link
Member

@vojtechjelinek vojtechjelinek Aug 5, 2020

Choose a reason for hiding this comment

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

Could you please introduce a controller then?

Copy link
Member

@vojtechjelinek vojtechjelinek Aug 5, 2020

Choose a reason for hiding this comment

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

Or somehow manage it so that there is functionality similar to this piece of code.

Copy link
Contributor Author

@nishantwrp nishantwrp Aug 5, 2020

Choose a reason for hiding this comment

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

done. added an angular component. Also fixed the smooth scroll :)

before
before (1)

after
after (1)

Copy link
Member

@vojtechjelinek vojtechjelinek Aug 7, 2020

Choose a reason for hiding this comment

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

Thanks! It looks great now!

core/templates/pages/footer_js_libs.html Show resolved Hide resolved
@@ -106,6 +106,7 @@
/core/templates/pages/privacy-page/privacy-page.mainpage.html @seanlip
/core/templates/pages/terms-page/ @DubeySandeep
/core/templates/pages/terms-page/terms-page.mainpage.html @seanlip
Copy link
Member

@seanlip seanlip Aug 6, 2020

Choose a reason for hiding this comment

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

Do I need to be on this one? Probably not because it hasn't got any of the actual content?

Copy link
Member

@seanlip seanlip Aug 6, 2020

Choose a reason for hiding this comment

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

Also just to check -- why is this different for the privacy vs the terms page? I.e. why is one changing but not the other?

Copy link
Contributor Author

@nishantwrp nishantwrp Aug 7, 2020

Choose a reason for hiding this comment

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

Do I need to be on this one? Probably not because it hasn't got any of the actual content?

ok, removed.

Copy link
Contributor Author

@nishantwrp nishantwrp Aug 7, 2020

Choose a reason for hiding this comment

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

is this different for the privacy vs the terms page? I.e. why is one changing but not the other?

because terms page had jquery code. (Refer #10141 (comment))

Copy link
Member

@seanlip seanlip Aug 7, 2020

Choose a reason for hiding this comment

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

Ah I see. OK, thanks!

@@ -106,6 +106,7 @@
/core/templates/pages/privacy-page/privacy-page.mainpage.html @seanlip
/core/templates/pages/terms-page/ @DubeySandeep
/core/templates/pages/terms-page/terms-page.mainpage.html @seanlip
Copy link
Member

@seanlip seanlip Aug 6, 2020

Choose a reason for hiding this comment

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

Also just to check -- why is this different for the privacy vs the terms page? I.e. why is one changing but not the other?

@seanlip seanlip assigned nishantwrp and unassigned seanlip Aug 6, 2020
@nishantwrp nishantwrp assigned seanlip and unassigned nishantwrp Aug 7, 2020
@nishantwrp nishantwrp requested a review from seanlip Aug 7, 2020
seanlip
seanlip approved these changes Aug 7, 2020
@seanlip seanlip removed their assignment Aug 7, 2020
Copy link
Contributor

@sagangwee sagangwee left a comment

@nishantwrp LGTM for codeowners. Thanks!

@sagangwee sagangwee removed their assignment Aug 7, 2020
Copy link
Member

@kevintab95 kevintab95 left a comment

LGTM, thanks @nishantwrp!

@kevintab95 kevintab95 removed their assignment Aug 7, 2020
aks681
aks681 approved these changes Aug 7, 2020
Copy link
Member

@aks681 aks681 left a comment

Lgtm for codeowner files

@oppiabot
Copy link

oppiabot bot commented Aug 7, 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!

@nishantwrp
Copy link
Contributor Author

nishantwrp commented Aug 8, 2020

@DubeySandeep ptal!

Copy link
Member

@DubeySandeep DubeySandeep left a comment

LGTM!

@DubeySandeep DubeySandeep merged commit 705a508 into oppia:develop Aug 8, 2020
6 checks passed
@nishantwrp nishantwrp deleted the header-imports branch Aug 8, 2020
shavavo pushed a commit to shavavo/oppia that referenced this pull request Aug 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment