#833 feat(service-details): add quick navigation menu anchors #875

eliat123 · 2017-05-23T10:54:27Z

Signed-off-by: Eli Atzaba eliat123@gmail.com
Fix #833

thojkooi · 2017-05-23T13:52:40Z

Doesn't this result in a page refresh of the service details? Which means that if you are editing some fields and click a link in the nav menu, you will lose your changes?

deviantony · 2017-05-23T18:55:40Z

Indeed, as @Glowbal stated, this introduces loss in the changes you bring in the form if you click on a menu link.

I think that this should be done differently, maybe if you keep using the goToItem method on each link, but set the anchor in that method?

eliat123 · 2017-05-23T20:42:44Z

app/components/service/serviceController.js

@@ -39,6 +39,8 @@ function ($scope, $stateParams, $state, $location, $anchorScroll, Service, Servi
  };

  $scope.goToItem = function(hash) {
+    $state.params.page_anchor = hash;


a hack to avoid full page refresh when changing $location.path

eliat123 · 2017-05-23T20:44:10Z

@Glowbal @deviantony updated without full page refresh, please review.

deviantony · 2017-05-24T06:53:51Z

@eliat123 could you rebase your changes? 1.13.0 was released yesterday and #868 introduced a lot of changes.

eliat123 · 2017-05-24T14:22:11Z

@deviantony rebased to latest develop, please review.

deviantony

I don't like the current implementation, can you have a look at my comments?

deviantony · 2017-05-25T09:31:12Z

app/app.js

@@ -428,7 +428,7 @@ angular.module('portainer', [
      }
    })
    .state('service', {
-      url: '^/service/:id/',
+      url: '^/service/:id/:page_anchor',


I'm not fond of having to add an extra :page_anchor parameter here.

@deviantony In my implementation you can have a direct access to a page anchor.

What would be the url format without an additional parameter?

Sorry for my late response.

I don't think there is a need for a direct access to a page anchor, what would be the goal here? Who's going to type the full URL to a service (including the service ID) ?

I also think that location.hash(hash) set the hash in the URL, we might need to add id on elements in the page though.

@deviantony I think that users can use this url as a bookmark or as #833 requested.

Why would we want to prevent this usage from users?

@eliat123 yes I agree but you do not need to add this as a state parameter. You can set the hash in the URL with $location.hash(<HASH>) and then call $anchorScroll() to automatically scroll to that hash.

Note that we will need to add id= on each HTML component that we want to be able to scroll to (e.g. networks, ports, etc...)

See my comment below for a code example.

@deviantony I do not fully understand what you mean, just as an example, the url might be:
http://127.0.0.1:9000/#/service/m132l2db91jg31txe9i8s6f1u-service-tasks
If not, please provide an example.

Ok have a look at b42272c

This is an example on how to use $anchorScroll() to automatically scrolls to element with IDs.

This way you can use: http://portainer.domain:9000/#/service/ID/#networks to access networks for example.

And then you can use $location.hash('networks') to scroll to the network element programmatically.

Ok, I will try to find time to work on it.

So I've been able to test it again, it's working well. But could you remove the :page_anchor parameter from the service state?

deviantony · 2017-05-25T09:35:00Z

app/components/service/serviceController.js

@@ -37,6 +37,8 @@ function ($q, $scope, $stateParams, $state, $location, $anchorScroll, ServiceSer
  };

  $scope.goToItem = function(hash) {
+    $state.params.page_anchor = hash;
+    $location.path('/service/'+$scope.service.Id+'/'+hash);


Instead of using this hack and $location.path could we use $location.hash and $anchorScroll ?

Something like:

$scope.goToItem = function(hash) { var newHash = hash; if ($location.hash() !== newHash) { $location.hash(hash); } else { $anchorScroll(); } };

As presented in the second example here: https://docs.angularjs.org/api/ng/service/$anchorScroll

deviantony · 2017-05-25T09:36:32Z

app/components/service/serviceController.js

@@ -269,6 +272,10 @@ function ($q, $scope, $stateParams, $state, $location, $anchorScroll, ServiceSer
      $scope.service = service;
      ControllerDataPipeline.setAccessControlData('service', $stateParams.id, service.ResourceControl);
      originalService = angular.copy(service);
+
+      if(page_anchor) {


Then just call $anchorScroll() here instead of that condition, and it should automatically scroll the hash when the page is refreshed.

deviantony · 2017-05-28T07:06:03Z

ping @eliat123

eliat123 · 2017-05-29T07:40:43Z

app/components/service/serviceController.js

      $scope.secrets = data.secrets.map(function (secret) {
        return new SecretViewModel(secret);
      });
+
+      $timeout(function() {


the $anchorScroll has to occur after the page has been fully rendered, (otherwise the anchor doesn't exist). This can be achieved using $timeout()

I agree with that.

eliat123 · 2017-05-29T07:41:23Z

@deviantony I have rebased and added the changes, please review.

deviantony

Hi @eliat123

Sorry for the late update !

I did another review and some testing. Overall LGTM but you need to remove that page_anchor parameter from the service state as well as fix an identifier.

See my comments.

deviantony · 2017-06-17T13:47:57Z

app/app.js

@@ -428,7 +428,7 @@ angular.module('portainer', [
      }
    })
    .state('service', {
-      url: '^/service/:id/',
+      url: '^/service/:id/:page_anchor',


So I've been able to test it again, it's working well. But could you remove the :page_anchor parameter from the service state?

deviantony · 2017-06-17T13:50:26Z

app/components/service/serviceController.js

      $scope.secrets = data.secrets.map(function (secret) {
        return new SecretViewModel(secret);
      });
+
+      $timeout(function() {


I agree with that.

deviantony · 2017-06-17T13:52:19Z

app/components/service/includes/container-specs.html

@@ -1,4 +1,4 @@
-<div>
+<div id="service-resources">


The service-resources id should be placed on the service specification panel located in app/components/service/includes/resources.html instead of container-specs.html.

Signed-off-by: Eli Atzaba <eliat123@gmail.com>

eliat123 · 2017-06-18T06:34:41Z

@deviantony - I have fixed the requested changes, please review.

deviantony

LGTM

deviantony · 2017-06-20T10:54:33Z

Thanks @eliat123 👍

* fix(helm-rbac) check rbac on helm install * Use correct auth check

deviantony added contrib/func-review-approved labels May 23, 2017

deviantony added changes-required Waiting for an update of the contributor and removed contrib/tech-review-in-progress labels May 23, 2017

eliat123 force-pushed the fix833-Quick-navigation-menu-with-anchors branch from 144d00f to 05f487a Compare May 23, 2017 20:39

eliat123 commented May 23, 2017

View reviewed changes

deviantony added rebase-required Indicates that the PR must be rebased on the latest development branch and removed changes-required Waiting for an update of the contributor labels May 24, 2017

eliat123 force-pushed the fix833-Quick-navigation-menu-with-anchors branch from 05f487a to 7b02958 Compare May 24, 2017 14:15

deviantony added contrib/tech-review-in-progress and removed rebase-required Indicates that the PR must be rebased on the latest development branch labels May 24, 2017

deviantony requested changes May 25, 2017

View reviewed changes

deviantony added changes-required Waiting for an update of the contributor and removed contrib/tech-review-in-progress labels May 25, 2017

deviantony assigned eliat123 May 28, 2017

eliat123 force-pushed the fix833-Quick-navigation-menu-with-anchors branch 4 times, most recently from d94c3a7 to a4a0079 Compare May 29, 2017 07:37

eliat123 commented May 29, 2017

View reviewed changes

deviantony added contrib/tech-review-in-progress and removed changes-required Waiting for an update of the contributor labels May 29, 2017

deviantony requested changes Jun 17, 2017

View reviewed changes

deviantony added changes-required Waiting for an update of the contributor and removed contrib/tech-review-in-progress labels Jun 17, 2017

feat(service-details): add quick navigation menu anchors

485e6b4

Signed-off-by: Eli Atzaba <eliat123@gmail.com>

eliat123 force-pushed the fix833-Quick-navigation-menu-with-anchors branch from a4a0079 to 485e6b4 Compare June 18, 2017 06:31

deviantony approved these changes Jun 19, 2017

View reviewed changes

deviantony added contrib/tech-review-approved and removed contrib/func-review-approved changes-required Waiting for an update of the contributor labels Jun 19, 2017

deviantony merged commit 9360f24 into portainer:develop Jun 20, 2017

xAt0mZ pushed a commit that referenced this pull request Aug 25, 2022

fix(helm-rbac): check rbac on helm install EE-1802 (#875)

4d2e5d7

* fix(helm-rbac) check rbac on helm install * Use correct auth check

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#833 feat(service-details): add quick navigation menu anchors #875

#833 feat(service-details): add quick navigation menu anchors #875

eliat123 commented May 23, 2017 •

edited

thojkooi commented May 23, 2017

deviantony commented May 23, 2017

eliat123 May 23, 2017

eliat123 commented May 23, 2017

deviantony commented May 24, 2017

eliat123 commented May 24, 2017

deviantony left a comment

deviantony May 25, 2017

eliat123 May 25, 2017 •

edited

deviantony May 27, 2017 •

edited

eliat123 May 28, 2017

deviantony May 28, 2017 •

edited

eliat123 May 28, 2017

deviantony May 28, 2017

eliat123 May 28, 2017

deviantony Jun 17, 2017 •

edited

deviantony May 25, 2017

deviantony May 25, 2017

deviantony commented May 28, 2017

eliat123 May 29, 2017

deviantony Jun 17, 2017

eliat123 commented May 29, 2017

deviantony left a comment

deviantony Jun 17, 2017 •

edited

deviantony Jun 17, 2017

deviantony Jun 17, 2017

eliat123 commented Jun 18, 2017

deviantony left a comment

deviantony commented Jun 20, 2017

#833 feat(service-details): add quick navigation menu anchors #875

#833 feat(service-details): add quick navigation menu anchors #875

Conversation

eliat123 commented May 23, 2017 • edited

thojkooi commented May 23, 2017

deviantony commented May 23, 2017

Choose a reason for hiding this comment

eliat123 commented May 23, 2017

deviantony commented May 24, 2017

eliat123 commented May 24, 2017

deviantony left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eliat123 May 25, 2017 • edited

Choose a reason for hiding this comment

deviantony May 27, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deviantony May 28, 2017 • edited

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

deviantony Jun 17, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deviantony commented May 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eliat123 commented May 29, 2017

deviantony left a comment

Choose a reason for hiding this comment

deviantony Jun 17, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eliat123 commented Jun 18, 2017

deviantony left a comment

Choose a reason for hiding this comment

deviantony commented Jun 20, 2017

eliat123 commented May 23, 2017 •

edited

eliat123 May 25, 2017 •

edited

deviantony May 27, 2017 •

edited

deviantony May 28, 2017 •

edited

deviantony Jun 17, 2017 •

edited

deviantony Jun 17, 2017 •

edited