Skip to content

Conversation

@babokim
Copy link

@babokim babokim commented Nov 4, 2015

This PR is for read-only mode.

Copy link
Member

Choose a reason for hiding this comment

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

warning?

@babokim babokim force-pushed the readonly branch 2 times, most recently from 7024ee2 to b3fd8d7 Compare November 6, 2015 01:53
@r-kamath
Copy link
Member

r-kamath commented Nov 6, 2015

@babokim Thanks for the update. Looks good to me.

Copy link

Choose a reason for hiding this comment

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

I think this condition should be in runParagraph(...) too

Copy link
Member

Choose a reason for hiding this comment

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

@Xrixcis I think it is intended to make Zeppelin readonly(blocks notebook/paragraph CRUD), but let user still able to run.

@Xrixcis
Copy link

Xrixcis commented Nov 10, 2015

Hi, great feature, thanks!
One little bug I came across, isReadOnly function seems to be missing in paragraph.controller.js.

@babokim babokim force-pushed the readonly branch 3 times, most recently from 5e5f0b1 to 5c779c5 Compare November 12, 2015 09:57
Copy link
Member

Choose a reason for hiding this comment

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

Setting button in 'disabled' class doesn't look different much.
image

I think it's better hide the button.

@Leemoonsoo
Copy link
Member

Tested and working nicely.
Thanks @babokim for contributing really useful feature.

Looks good to me

Copy link
Member

Choose a reason for hiding this comment

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

I think we avoid this style kind of *. Could you please change this to separate libraries?

@Leemoonsoo
Copy link
Member

Merging if there're no more discussions.

@jongyoul
Copy link
Member

@Leemoonsoo I think we should take a care of a style of import *. What do you think of it? This looks minor for now but it needs to keep it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Duplicate with Line 33

@corneadoug
Copy link
Contributor

I'm a bit late for the party,
However I would prefer the isReadOnly() part in zeppelin-web to be handled as a DataFactory instead of storing it in the $rootScope and copy pasting the same function in every controllers.

There is a good example here:
https://github.com/apache/incubator-zeppelin/blob/master/zeppelin-web/src/components/notebookListDataFactory/notebookList.datafactory.js

And you can see it in action here:
https://github.com/apache/incubator-zeppelin/blob/master/zeppelin-web/src/components/navbar/navbar.controller.js

@babokim
Copy link
Author

babokim commented Nov 17, 2015

@corneadoug Thank you for your advise. I tried storing system config values in a DataFactory. That way also has similar duplication. To set the data in that DataFactory after receiving from the Websocket, each controller should have receiver function like the following code. That function code also is duplicated in every controllers.

angular.module('zeppelinWebApp').factory('websocketEvents', function ( ... ) {
  websocketCalls.ws.onMessage(function(event) {
    ...
    } else if (op === 'GET_SYSTEM_CONF') {
      $rootScope.$broadcast('setSystemConf', data.conf);
    }
  }
}

angular.module('zeppelinWebApp').controller('NotebookCtrl'..., function(...)  {
  $scope.$on('setSystemConf', function(event, systemConf  {
    systemConfigDataFactory.setSystemConf(systemConf);
  });

If there is another way to set system config data, please let me know. Thanks.

@corneadoug
Copy link
Contributor

@babokim
That's not exactly how it should be used, the goal is to use it as a data storage.
Let's say you have a similar DataFactory as the one I gave in example:

angular.module('zeppelinWebApp').factory('systemConfDataFactory', function() {
  var vm = {};

  vm.conf = {};

  vm.setSystemConf = function(systemConf) {
    vm.conf = angular.copy(systemConf);
  };
  return vm;
});

Then you just have to call setSystemConf in your websocketEvents like this:

angular.module('zeppelinWebApp').factory('websocketEvents', function ( ...,  systemConfDataFactory) {
  websocketCalls.ws.onMessage(function(event) {
    ...
    } else if (op === 'GET_SYSTEM_CONF') {
      systemConfDataFactory.setSystemConf(data.conf);
    }
  }
}

And when you need to do some ng-if or ng-hide, you can do like this:
home.controller

 angular.module('zeppelinWebApp').controller('HomeCtrl', function(..., systemConfDataFactory) {
   vm.systemConfDataFactory = systemConfDataFactory;

home.html


<h5 ng-hide="home.systemConfDataFactory.conf.readonly">
  <a href="" data-toggle="modal" data-target="#noteNameModal" style="text-decoration: none;">
    <i style="font-size: 15px;" class="icon-notebook"></i> Create new note</a>
</h5>

@babokim
Copy link
Author

babokim commented Nov 19, 2015

@corneadoug Thank you for your kindness. I will try as your way.

@babokim babokim force-pushed the readonly branch 5 times, most recently from a5dd56e to 0037f80 Compare November 26, 2015 00:38
@Leemoonsoo
Copy link
Member

@babokim Do you mind rebase or merge master to resolve the conflicts?

@rishitesh
Copy link

@babokim , Can you please merge this feature ?

@rishitesh
Copy link

Its really a useful feature for demo , where multiple users might try their hands.

@felixcheung
Copy link
Member

@babokim - would you be able to update this PR? It looks like the community would appreciate your contributor to make this possible ;)

@corneadoug
Copy link
Contributor

I can also make a PR to this one to fix the zeppelin-web part if you don't have the time

@dusenberrymw
Copy link

Agree with @rishitesh that this would be really great for live demos!

@felixcheung
Copy link
Member

@babokim haven't heard from you - would you be able to continue this work?

@FRosner
Copy link
Contributor

FRosner commented Apr 8, 2016

Thumbs up also from my side if we can resolve conflicts, finish discussions and merge if appropriate 👍

@nolangrace
Copy link

I would love to take advantage of this feature is there anything I can do help move this pull request forward?

@meniluca
Copy link
Contributor

Ditto! :) 👍

@corneadoug
Copy link
Contributor

@babokim
Going around old PRs,
Is it still needed considering the efforts for user authentication, and notebook acl restrictions?

@asfgit asfgit closed this in c38a0a0 May 9, 2018
asfgit pushed a commit that referenced this pull request May 9, 2018
close #83
close #86
close #125
close #133
close #139
close #146
close #193
close #203
close #246
close #262
close #264
close #273
close #291
close #299
close #320
close #347
close #389
close #413
close #423
close #543
close #560
close #658
close #670
close #728
close #765
close #777
close #782
close #783
close #812
close #822
close #841
close #843
close #878
close #884
close #918
close #989
close #1076
close #1135
close #1187
close #1231
close #1304
close #1316
close #1361
close #1385
close #1390
close #1414
close #1422
close #1425
close #1447
close #1458
close #1466
close #1485
close #1492
close #1495
close #1497
close #1536
close #1545
close #1561
close #1577
close #1600
close #1603
close #1678
close #1695
close #1739
close #1748
close #1765
close #1767
close #1776
close #1783
close #1799
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.