Skip to content

Conversation

@astroshim
Copy link
Contributor

What is this PR for?

It needs to filtering subdirectory names in navbar.

What type of PR is it?

Bug Fix

What is the Jira issue?

https://issues.apache.org/jira/browse/ZEPPELIN-886

How should this be tested?

try filter the note name on navbar.

Screenshots (if appropriate)

gif

Questions:

  • Does the licenses files need update? no
  • Is there breaking changes for older versions? no
  • Does this needs documentation? no

@cloverhearts
Copy link
Member

Looks good to me +1

return notebook;
}

if(notebook.children) {
Copy link
Member

Choose a reason for hiding this comment

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

style: extra space if (?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh I see. thank you for your comment @felixcheung

<li><a href="" data-toggle="modal" data-target="#noteNameModal"><i class="fa fa-plus"></i> Create new note</a></li>
<li class="divider"></li>
<div id="notebook-list" class="scrollbar-container">
<li class="filter-names" ng-include="'components/filterNoteNames/filter-note-names.html'"></li>
Copy link
Contributor

Choose a reason for hiding this comment

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

We should make keep that line, and modify filter-note-names.html to have ng-model="$parent.query"

@corneadoug
Copy link
Contributor

@astroshim Tested, works well, except for those 2 comments LGTM.

The code handling those notebook lists and their filtering became quite messy and error prone.
I will take some time to refactor it later.

@astroshim
Copy link
Contributor Author

@corneadoug Thank you for your review.
I'll refactor the code as you mentioned.

@astroshim
Copy link
Contributor Author

@corneadoug I refactored the codes that you pointed out. please review.

@corneadoug
Copy link
Contributor

Tested, Merging if no more discussions

@asfgit asfgit closed this in aeb34fb Jun 14, 2016
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.

4 participants