Skip to content

Conversation

@zhongneu
Copy link
Contributor

@zhongneu zhongneu commented Mar 8, 2016

What is this PR for?

This PR is based on #190

What type of PR is it?

Feature

Todos

  • - Should disable clicking the folder in dropdown menu
  • - CSS cleanups
  • - Add instruction about how to add folder in the notebook create menu
  • - Sort the notes / dirs by name
  • - The hidden property is shared across clients?

What is the Jira issue?

ZEPPELIN-209

How should this be tested?

Screenshots (if appropriate)

cwmupp5z42

Questions:

  • Does the licenses files need update?

NO

  • Is there breaking changes for older versions?

NO

  • Does this needs documentation?

NO?

@zhongneu
Copy link
Contributor Author

zhongneu commented Mar 8, 2016

@corneadoug I've created a new pr here. Thanks for your review!

@bzz
Copy link
Member

bzz commented Mar 9, 2016

@zhongneu thank you for taking a stab at bringing that feature to life again!

👍 for screenshots, but could you please also updated PR description a bit, explaining how exactly the hierarchy is formed? Like how to create a nested structure of notebooks, and if there are any changes in how notes are persisted.

This, as well as screenshots, usually helps communicating the value of the feature and gauge people's the interest.

Also please feel free to assign ZEPPELIN-209 to yourself, so it's not get lost on the release.

@bzz
Copy link
Member

bzz commented Mar 9, 2016

\cc @felizbear for a reivew

@corneadoug
Copy link
Contributor

I didn't test it yet, but I also think it could be nice to have feedbacks from people who worked on some Notebook storages. (Git, S3 etc..) @vgmartinez @khalidhuseynov

@Leemoonsoo
Copy link
Member

I think way it implements is just treating '/' in the notebook name as a directory separator in a front end side as described in ZEPPELIN-209. and it is not really related to the notebook storage.

};

$scope.toggleFolderNode = function(node) {
node.hidden = node.hidden ? false : true;
Copy link
Contributor

Choose a reason for hiding this comment

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

node.hidden = !node.hidden

improves readability / understandability :)

@prabhjyotsingh
Copy link
Contributor

Any thoughts about; showing folders first and then files ?


notes.setNotes = function(notesList) {
notes.list = angular.copy(notesList);
notes.flatList = angular.copy(notesList); // a flat list to boost searching
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not convinced that representing a hierarchy with lists is such a good idea. Why not use objects instead? Then we can rely on lodash's set and get, like this:

// set notes
    var notes = notesList.reduce(function(acc, note) {
      var noteName = note.name || note.id;
      var dirs = noteName.match(/([^\\\][^\/]|\\\/)+/g);

      return _.set(
        acc,
        dirs.join("."),
        {id: note.id}
      )
    }, {})

// get a note at "/C/CB/CBA"
  _.get(notes, "C.CB.CBA")

What do you think?

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.

6 participants