[ZEPPELIN-869] Search notebook to handle empty result. by rranjan03 · Pull Request #978 · apache/zeppelin

rranjan03 · 2016-06-09T10:16:56Z

What is this PR for?

Search notebook to handle empty result.

What type of PR is it?

Bug Fix

Todos

What is the Jira issue?

ZEPPELIN-869

How should this be tested?

Try to search something which should return empty result, you should get proper message for that.
search box should get cleared when clicked on any other link.

Screenshots (if appropriate)

Before:

After:

Questions:

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

bzz · 2016-06-09T10:22:07Z

@ravicodder thank you for improvement!

\cc @felizbear as an original author of search frontend for a review

prabhjyotsingh · 2016-06-10T17:25:34Z

LGTM, I noticed you are clearing search box on click of any link 👍

heruka-urgyen · 2016-06-12T07:14:19Z

+    var routeChangeEvent = $rootScope.$on('$routeChangeStart', function (event, next, current) {
+      if (next.originalPath !== '/search/:searchTerm') {
+        angular.element('#searchTermId').val('');
+        routeChangeEvent();


I wonder, why do you recursively call routeChangeEvent here?

@felizbear by calling routeChangeEvent I am explicitly unbinding the event handler.
$routeScope.$on returns a deregistration function whose reference we are storing in routeChangeEvent.

Maybe it makes sense to rename routeChangeEvent into something like unbindRouteChangeEvent? with such name no questions would arise about why it is invoked

@felizbear ya, it makes sense. Renamed routeChangeEvent to unbindRouteChangeEvent.

heruka-urgyen · 2016-06-12T07:27:47Z

Also, I would recommend modify ng-disabled of search form in navbar like this:

ng-disabled="!navbar.connected || !searchTerm"

so it doesn't fire when there is an empty field

heruka-urgyen · 2016-06-12T07:31:18Z

Also, I think you will have to clean up scope after you clear the search field value. To see what I mean do the following:

Search for something, e.g. bankText
Navigate away from the search result page (search field now is empty)
Search again with the empty search field
See bankText results again (expected to see an empty page or even have search field disabled like I suggested above)

heruka-urgyen · 2016-06-12T07:31:32Z

Thanks for contribution by the way :)

rranjan03 · 2016-06-13T06:01:23Z

Hi @felizbear thanks for the review.
I have done the required changes, Please have a look.

…into ZEPPELIN-869 Conflicts: zeppelin-web/src/components/navbar/navbar.html

heruka-urgyen · 2016-06-13T07:50:01Z

@ravicodder except for this remark, LGTM

corneadoug · 2016-06-13T08:34:14Z

+      $scope.isResult = true;
+    }
+
+    var routeChangeEvent = $rootScope.$on('$routeChangeStart', function (event, next, current) {


This should be $scope.$on

corneadoug · 2016-06-13T08:35:51Z

The searchTerm would probably be better inside the searchService instead of the $rootScope

rranjan03 · 2016-06-13T12:16:51Z

@corneadoug Thanks for the review. Done required changes, kindly review it.

…into ZEPPELIN-869 Conflicts: zeppelin-web/src/components/navbar/navbar.controller.js

corneadoug · 2016-06-14T05:36:50Z

    });
  };

+  this.searchTerm = '';


You will need to do a this.searchTerm = term.q; at the beginning of the this.search function, otherwise there is no search field when you reload the page

corneadoug · 2016-06-14T05:38:57Z

@ravicodder Thank you for rebasing, I left a few more comments

rranjan03 · 2016-06-14T10:21:50Z

@corneadoug done required changes.
Please, have a look.

prabhjyotsingh · 2016-06-14T10:25:50Z

 angular
  .module('zeppelinWebApp')
-  .controller('SearchResultCtrl', function($scope, $routeParams, searchService) {
+  .controller('SearchResultCtrl', function($scope, $routeParams, searchService, $rootScope) {


I think now $rootScope can be removed, its unused.

@prabhjyotsingh removed $rootScope in latest commit.

…tion remove rootscope

Handle empty search

7315707

rranjan03 force-pushed the ZEPPELIN-869 branch from 152efbb to 7315707 Compare June 9, 2016 10:59

heruka-urgyen reviewed Jun 12, 2016
View reviewed changes

add check on searchTerm and clan up scope

5d5a072

rranjan03 force-pushed the ZEPPELIN-869 branch from 022c50d to 0fa3b2e Compare June 13, 2016 06:30

Merge branch 'master' of https://github.com/apache/incubator-zeppelin …

d2f66a7

…into ZEPPELIN-869 Conflicts: zeppelin-web/src/components/navbar/navbar.html

rranjan03 force-pushed the ZEPPELIN-869 branch from 0fa3b2e to d2f66a7 Compare June 13, 2016 07:13

corneadoug reviewed Jun 13, 2016
View reviewed changes

use searchServise in place of rootscope

6c3db2c

Merge branch 'master' of https://github.com/apache/incubator-zeppelin …

7096781

…into ZEPPELIN-869 Conflicts: zeppelin-web/src/components/navbar/navbar.controller.js

corneadoug reviewed Jun 14, 2016
View reviewed changes

rranjan03 force-pushed the ZEPPELIN-869 branch from 6da2b0c to 28fab0a Compare June 14, 2016 09:48

prabhjyotsingh reviewed Jun 14, 2016
View reviewed changes

rranjan03 force-pushed the ZEPPELIN-869 branch 2 times, most recently from 3f62ed6 to be51289 Compare June 14, 2016 11:37

Remove controller , add update searchTerm, remove deregistration func…

cab339d

…tion remove rootscope

Conversation

rranjan03 commented Jun 9, 2016

What is this PR for?

What type of PR is it?

Todos

What is the Jira issue?

How should this be tested?

Screenshots (if appropriate)

Questions:

Uh oh!

bzz commented Jun 9, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

prabhjyotsingh commented Jun 10, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

heruka-urgyen commented Jun 12, 2016

Uh oh!

heruka-urgyen commented Jun 12, 2016

Uh oh!

heruka-urgyen commented Jun 12, 2016

Uh oh!

rranjan03 commented Jun 13, 2016

Uh oh!

heruka-urgyen commented Jun 13, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

corneadoug commented Jun 13, 2016

Uh oh!

rranjan03 commented Jun 13, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

corneadoug commented Jun 14, 2016

Uh oh!

rranjan03 commented Jun 14, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

bzz commented Jun 9, 2016 •

edited

Loading

prabhjyotsingh commented Jun 10, 2016 •

edited

Loading