ZEPPELIN-866] Hotfix - does not working search box on navbar. by cloverhearts · Pull Request #911 · apache/zeppelin

cloverhearts · 2016-05-23T11:30:36Z

What is this PR for?

When you type in the search box and start the search,
The result is not exposed.
Previously, it was working well.

What type of PR is it?

Hot Fix

Todos

- fixed bug on navbar.html and controller

What is the Jira issue?

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

How should this be tested?

try to search action on navbar.

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

prabhjyotsingh · 2016-05-23T11:51:28Z

- $scope.search = function() {
- $location.url(/search/ + $scope.searchTerm);
+ $scope.search = function(searchTerm) {
+ console.log('search term ', searchTerm);


Can you remove console.log from here.

@prabhjyotsingh oh, Thank you!

prabhjyotsingh · 2016-05-23T11:55:21Z

If we use $scope.search = function(searchTerm) then we can this function becomes redundant. Can you remove this as well.

$rootScope.$on('$locationChangeSuccess', function () {
var path = $location.path();
// hacky solution to clear search bar
// TODO(felizbear): figure out how to make ng-click work in navbar
if (path === '/') {
$scope.searchTerm = '';
}
});

cloverhearts · 2016-05-23T12:04:35Z

@prabhjyotsingh
I think that code would clean up the search box.
It should not be deleted.

prabhjyotsingh · 2016-05-23T12:06:19Z

Or that is also broken for $locationChangeSuccess, its not cleaning up. Can you fix this as well ?

cloverhearts · 2016-05-23T12:09:19Z

@prabhjyotsingh Okay, i will look to this issue.

cloverhearts · 2016-05-23T12:51:29Z

@prabhjyotsingh
you're alright.
this is code is bug and dead code. If active to code, the search becomes impossible.
So I have deleted that code.

cloverhearts · 2016-05-24T02:06:05Z

@prabhjyotsingh
fix done.
Please, Can you feedback for this pr for me?

bzz · 2016-05-24T02:36:30Z

Thanks for hotfix, but do we know why it stopped working in the first place?
Let me take a look.

\cc @felizbear for review as well

astroshim · 2016-05-24T02:39:40Z

I tesed and working well. but it seems need a "initial statement".

heruka-urgyen · 2016-05-24T02:41:15Z

 <form role="search"
 style="width: 300px; display: inline-block; margin: 0 10px"
- ng-submit="search()">
+ ng-submit="search(searchTerm)">


i definitely like passing the argument to search instead of taking it from the $scope 👍

cloverhearts · 2016-05-24T02:47:27Z

@astroshim Thank you for feed back.
are you meant 'initial statement' for 'clean up on search box'?

AhyoungRyu · 2016-05-24T03:16:35Z

@cloverhearts I agree with @astroshim. I think he was saying that when the result is nothing, Zeppelin needs to show some statement like "We couldn’t find any notebook matching 'something_you_typed' "

astroshim · 2016-05-24T03:22:04Z

Thanks for the detail explanation AhyoungRyu.

cloverhearts · 2016-05-24T05:27:13Z

Thank you for explaining, @astroshim @AhyoungRyu
I agree too.
But, This is must exist only for bug fixes at this PR.
I will make it in the next PR.

AhyoungRyu · 2016-05-24T05:29:00Z

@cloverhearts Yeah you're right. LGTM 👍

prabhjyotsingh · 2016-05-24T05:32:54Z

I think this sounds good, have created a jira for the same https://issues.apache.org/jira/browse/ZEPPELIN-869. LGTM.
I'll Merge this if no more discussion.

bzz · 2016-05-24T05:54:32Z

Let me take a look.

@prabhjyotsingh could you please wait, as there was an issue raised, but not addressed yet?

Thanks for hotfix, but do we know why it stopped working in the first place?

It would be really cool, if we could know the answer, as I think it might help us avoiding such bugs further down the road. @cloverhearts could you please share, if you have already done some investigation before?

prabhjyotsingh · 2016-05-24T05:56:38Z

Surething.

cloverhearts · 2016-05-25T05:15:02Z

@bzz
can not access to '$ scope.searchTerm variable at navbar.html' in navbar.controller
Code and nav are not currently operating normally.
I did not find the PR of the problem, which seems to be a problem at some point.
This can cause a lot of problems in the current navbar.
For example
#912

I think we need to recover the ability to hotfix first.
And it should retouching the navbar.

bzz · 2016-05-25T11:26:41Z

@cloverhearts fix looks great to me. Pity that we can not identify the source of the bug though.

Let's merge as a hotfix!

prabhjyotsingh · 2016-05-25T11:45:16Z

Sure, I'll merge this.

### What is this PR for? When you type in the search box and start the search, The result is not exposed. Previously, it was working well. ### What type of PR is it? Hot Fix ### Todos * [x] - fixed bug on navbar.html and controller ### What is the Jira issue? https://issues.apache.org/jira/browse/ZEPPELIN-866 ### How should this be tested? try to search action on navbar. ### Screenshots (if appropriate) #### before <img width="1280" alt="test02" src="https://cloud.githubusercontent.com/assets/10525473/15469402/10713c64-2125-11e6-909d-cb375e7c31a4.png"> #### after <img width="1280" alt="test01" src="https://cloud.githubusercontent.com/assets/10525473/15469411/1a6e0b0c-2125-11e6-9ade-e4147e7143d8.png"> ### Questions: * Does the licenses files need update? no * Is there breaking changes for older versions? no * Does this needs documentation? no Author: CloverHearts <cloverheartsdev@gmail.com> Closesapache#911 from cloverhearts/fixed/searchbar and squashes the following commits: bca027b [CloverHearts] remove whitespace c592422 [CloverHearts] add space in navbar.controller.js 433139a [CloverHearts] removed event locationChangeSuccess on pr ( does not working search bar) bac1254 [CloverHearts] removed console.log (pr does not working search box) 4d06560 [CloverHearts] Hotfix - dose not working search box on navbar.

Hotfix - dose not working search box on navbar.
4d06560

prabhjyotsingh reviewed May 23, 2016
View reviewed changes

removed console.log (pr does not working search box)
bac1254

removed event locationChangeSuccess on pr ( does not working search bar)
433139a

cloverhearts added 2 commits May 23, 2016 23:07

add space in navbar.controller.js
c592422

remove whitespace
bca027b

heruka-urgyen reviewed May 24, 2016
View reviewed changes

asfgit closed this in 3b40c3aMay 26, 2016

cloverhearts deleted the fixed/searchbar branch June 16, 2016 06:25

Uh oh!

Conversation

cloverhearts commented May 23, 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)

before

Uh oh!

prabhjyotsinghMay 23, 2016

Choose a reason for hiding this comment

Uh oh!

cloverheartsMay 23, 2016

Choose a reason for hiding this comment

Uh oh!

prabhjyotsingh commented May 23, 2016

Uh oh!

cloverhearts commented May 23, 2016

Uh oh!

prabhjyotsingh commented May 23, 2016

Uh oh!

cloverhearts commented May 23, 2016

Uh oh!

cloverhearts commented May 23, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cloverhearts commented May 24, 2016

Uh oh!

bzz commented May 24, 2016

Uh oh!

astroshim commented May 24, 2016

Uh oh!

heruka-urgyenMay 24, 2016

Choose a reason for hiding this comment

Uh oh!

cloverhearts commented May 24, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AhyoungRyu commented May 24, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

astroshim commented May 24, 2016

Uh oh!

cloverhearts commented May 24, 2016

Uh oh!

AhyoungRyu commented May 24, 2016

Uh oh!

prabhjyotsingh commented May 24, 2016

Uh oh!

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

Uh oh!

Uh oh!

prabhjyotsingh commented May 24, 2016

Uh oh!

cloverhearts commented May 25, 2016

Uh oh!

bzz commented May 25, 2016

Uh oh!

prabhjyotsingh commented May 25, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

cloverhearts commented May 23, 2016•
edited
Loading

cloverhearts commented May 24, 2016•
edited
Loading

AhyoungRyu commented May 24, 2016•
edited
Loading

bzz commented May 24, 2016•
edited
Loading