Fix part of #5002: Removed NAV_MODE from side navigation bar #6008

mighty-phoenix · 2018-12-19T17:13:58Z

Explanation

Fixes part of #5002: Removed the constant NAV_MODE and made a new variable CURRENT_URL which stores the url of the current URL(as the name suggests). This variable is then used to determine the active tab/page.

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 PR explanation includes the words "Fixes #bugnum: ..." (or "Fixes part of #bugnum" if the PR only partially fixes an issue).
The linter/Karma presubmit checks have passed.
- These should run automatically, but if not, you can manually trigger them locally using python scripts/pre_commit_linter.py and bash scripts/run_frontend_tests.sh.
The PR is made from a branch that's not called "develop".
The PR follows the style guide.
The PR is assigned to an appropriate reviewer.
- If you're a new contributor, please ask on Gitter for someone to assign a reviewer.
- If you're not sure who the appropriate reviewer is, please assign to the issue's "owner" -- see the "talk-to" label on the issue.

mighty-phoenix · 2018-12-19T17:14:49Z

@vojtechjelinek
I have tested the changes and it's working pretty fine. Kindly review.

vojtechjelinek

Thanks! NAV_MODE is also used in TopNavigationBarDirective.js remove it from there too.

And after you remove all the usage of NAV_MODE in frontend you should remove it from our codebase completely. So for example from base.html, and in feconf.py there are constants related to NAV_MODE remove them too.

vojtechjelinek · 2018-12-19T17:27:44Z

core/templates/dev/head/components/side_navigation_bar/SideNavigationBarDirective.js

@@ -26,7 +26,7 @@ oppia.directive('sideNavigationBar', [
        'side_navigation_bar_directive.html'),
      controller: ['$scope', '$timeout', function(
          $scope, $timeout) {
-        $scope.NAV_MODE = GLOBALS.NAV_MODE;
+        $scope.CURRENT_URL = window.location.pathname;


Use angular version of window.location.pathname an since now it's not really a constant use camelCase

Suggested change

$scope.CURRENT_URL = window.location.pathname;

$scope.currentUrl = $location.path();

@vojtechjelinek after this change in SideNavigationBarDirective.js, the side nav bar has stopped working even after making equivalent changes in html. I also added '$location' to controller and function parameters. Still the expected behaviour is not established.

I'm not sure, try to debug it, is there some error in browser console, try to print the value of currentUrl to check it contains correct value.

@vojtechjelinek
I did all the research possible, and fixed the issue.

But now, a new problem has arised. The $location api doesn't have the capability of reloading the entire page, unlike window.location(Look at the screenshot below). So the url gets changed on clicking a button on side nav bar, but page doesn't get reloaded, resulting the user to stay on the same page as before. What should we do now? Use window.location itself?

Oh I see, we definitely shouldn't modify $locationProvider, so use window.location.pathname please.

@vojtechjelinek Alright. Thanks.

…igationBarDirective.js Co-Authored-By: rakshitkumarcse <32777813+rakshitkumarcse@users.noreply.github.com>

codecov-io · 2018-12-19T18:13:44Z

Codecov Report

Merging #6008 into develop will not change coverage.
The diff coverage is 0%.

@@           Coverage Diff           @@
##           develop   #6008   +/-   ##
=======================================
  Coverage     45.3%   45.3%           
=======================================
  Files          520     520           
  Lines        30667   30667           
  Branches      4597    4597           
=======================================
  Hits         13893   13893           
  Misses       16774   16774

Impacted Files	Coverage Δ
.../side_navigation_bar/SideNavigationBarDirective.js	`16.66% <0%> (ø)`	⬆️
...ts/top_navigation_bar/TopNavigationBarDirective.js	`0.8% <0%> (ø)`	⬆️
...xploration_player/ExplorationPlayerStateService.js	`1.53% <0%> (ø)`	⬆️

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 05ed54f...4a09285. Read the comment docs.

mighty-phoenix · 2018-12-19T18:14:41Z

@vojtechjelinek thanks for the guidance. This will make it easier for me to work on other parts of the issue.
Working on the changes.

mighty-phoenix · 2018-12-21T19:58:54Z

@vojtechjelinek The PR is ready for final review and merge.
Thanks!

vojtechjelinek

Hi, just one comment and nav_mode is also used in base.html so remove it from there.

vojtechjelinek · 2018-12-24T12:04:08Z

core/controllers/collection_editor.py

@@ -61,7 +61,7 @@ def get(self, collection_id):

        self.values.update({
            'collection_id': collection.id,
-            'nav_mode': feconf.NAV_MODE_CREATE,
+            'nav_mode': 'create',


Do you need this variable anymore? This is just passed to the Jinja and Jinja will include int the HTML files. And since you removed NAV_MODE from base.html it!s not needed anymore. Ditto for the other files below.

@vojtechjelinek Oh. Removing these then.
Thanks.

@vojtechjelinek Does this look fine now?

vojtechjelinek

Thanks. LGTM!

mighty-phoenix · 2018-12-24T18:23:33Z

@vojtechjelinek Kindly merge.

vojtechjelinek · 2018-12-24T20:36:15Z

@seanlip or @apb7 Review, please.

apb7

LGTM (for changes in core/tests/..), thanks @rakshitkumarcse!

mighty-phoenix · 2018-12-24T21:37:34Z

@apb7 Thanks for the review.

Fix part of oppia#5002: Removed NAV_MODE from side navigation bar

8780fae

vojtechjelinek suggested changes Dec 19, 2018

View reviewed changes

Update core/templates/dev/head/components/side_navigation_bar/SideNav…

420777c

…igationBarDirective.js Co-Authored-By: rakshitkumarcse <32777813+rakshitkumarcse@users.noreply.github.com>

mighty-phoenix added 3 commits December 21, 2018 02:33

Removed NAV_MODE successfully from side navbar

a0eb9a4

Removed NAV_MODE from top nav bar

52c00b6

Removed NAV_MODE from base.html

2520924

mighty-phoenix requested review from apb7 and seanlip as code owners December 21, 2018 17:54

Removed all constants related to NAV_MODE

f9cb363

vojtechjelinek suggested changes Dec 24, 2018

View reviewed changes

Correction

4a09285

vojtechjelinek approved these changes Dec 24, 2018

View reviewed changes

apb7 approved these changes Dec 24, 2018

View reviewed changes

apb7 merged commit b27bb32 into oppia:develop Dec 24, 2018

mighty-phoenix deleted the remove_globals branch December 24, 2018 21:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix part of #5002: Removed NAV_MODE from side navigation bar #6008

Fix part of #5002: Removed NAV_MODE from side navigation bar #6008

mighty-phoenix commented Dec 19, 2018 •

edited

Loading

mighty-phoenix commented Dec 19, 2018

vojtechjelinek left a comment

vojtechjelinek Dec 19, 2018

mighty-phoenix Dec 19, 2018

mighty-phoenix Dec 19, 2018 •

edited

Loading

vojtechjelinek Dec 19, 2018

mighty-phoenix Dec 20, 2018 •

edited

Loading

vojtechjelinek Dec 20, 2018

mighty-phoenix Dec 20, 2018

codecov-io commented Dec 19, 2018 •

edited

Loading

mighty-phoenix commented Dec 19, 2018

mighty-phoenix commented Dec 21, 2018

vojtechjelinek left a comment

vojtechjelinek Dec 24, 2018

mighty-phoenix Dec 24, 2018

mighty-phoenix Dec 24, 2018

vojtechjelinek Dec 24, 2018

vojtechjelinek left a comment

mighty-phoenix commented Dec 24, 2018

vojtechjelinek commented Dec 24, 2018

apb7 left a comment

mighty-phoenix commented Dec 24, 2018

	$scope.CURRENT_URL = window.location.pathname;
	$scope.currentUrl = $location.path();

Fix part of #5002: Removed NAV_MODE from side navigation bar #6008

Fix part of #5002: Removed NAV_MODE from side navigation bar #6008

Conversation

mighty-phoenix commented Dec 19, 2018 • edited Loading

Explanation

Checklist

mighty-phoenix commented Dec 19, 2018

vojtechjelinek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mighty-phoenix Dec 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mighty-phoenix Dec 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Dec 19, 2018 • edited Loading

Codecov Report

mighty-phoenix commented Dec 19, 2018

mighty-phoenix commented Dec 21, 2018

vojtechjelinek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vojtechjelinek left a comment

Choose a reason for hiding this comment

mighty-phoenix commented Dec 24, 2018

vojtechjelinek commented Dec 24, 2018

apb7 left a comment

Choose a reason for hiding this comment

mighty-phoenix commented Dec 24, 2018

mighty-phoenix commented Dec 19, 2018 •

edited

Loading

mighty-phoenix Dec 19, 2018 •

edited

Loading

mighty-phoenix Dec 20, 2018 •

edited

Loading

codecov-io commented Dec 19, 2018 •

edited

Loading