Skip to content

Add SearchBar for layers selection - #400

Merged
Ram81 merged 8 commits into
Cloud-CV:masterfrom
Jatana:layers-searchbar-v2
Oct 29, 2018
Merged

Add SearchBar for layers selection#400
Ram81 merged 8 commits into
Cloud-CV:masterfrom
Jatana:layers-searchbar-v2

Conversation

@Jatana

@Jatana Jatana commented Oct 27, 2018

Copy link
Copy Markdown
Contributor

ezgif-2-c00195b7c555

@thatbrguy thatbrguy left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Excellent work on the search bar! Looks neat. Just had a minor issue with webpack, which I have mentioned here.

Comment thread ide/static/js/pane.js
elem.style.display = 'none';
}
}
for (let elem of $('.panel-heading')) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is elem necessary here? As you have commented out the rest. (webpack error)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@thatbrguy elem is being used

@thatbrguy

Copy link
Copy Markdown
Contributor

@Ram81 @Jatana Looks good to me 👍 runs well locally.

Minor suggestions, add new lines to the end of your css files (the ones that doesn't have) and remove comments if you're sure about your code.

@Ram81 Ram81 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@Jatana few minor changes, Nice work overall.

Comment thread ide/static/css/searchbar_style.css Outdated

.matched-search-char {
color: rgb(69, 80, 97);
// text-decoration: underline;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If we dont need this please remove it.

Comment thread ide/static/css/searchbar_style.css Outdated
background: none;
border: none;
color: rgb(69, 80, 97);
// border-bottom: 1px solid black;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If we dont need this please remove it.

Comment thread ide/static/js/content.js Outdated
<h5 className="sidebar-heading">LOGIN</h5>
<Login setUserId={this.setUserId} setUserName={this.setUserName}></Login>
<h5 className="sidebar-heading">INSERT LAYER</h5>
<h5 className="sidebar-heading insert-layer-title"><input id="layer-search-input" placeholder="Search for layer"></input><div id="insert-layer-sign">INSERT LAYER</div><i className="material-icons" id="layer-search-icon">

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Align <div> and <input> tags, I mean add indentation and new lines to separate tags

@yashdusing

Copy link
Copy Markdown
Contributor

Looks good to me but I’ve encountered one bug. Whenever I type in a single letter (for eg : ‘r’) , many of the queries return false and it shows no layers as a result. I’ll add in a screenshot in a few minutes so please look into it. Overall, good work though !

@Ram81

Ram81 commented Oct 28, 2018

Copy link
Copy Markdown
Member

@yashdusing Nice catch. It happens every time there is only a single character in search. @Jatana can you fix the issue.

@gautamjajoo

Copy link
Copy Markdown
Member

@thatbrguy @Ram81 Are we allowed to work on two tasks at a single time because I have seen one more PR by @Jatana which are two differnet tasks. Just a clarification. Thanks! :)

@thatbrguy

Copy link
Copy Markdown
Contributor

@gautamjajoo The task pertinent to PR #394 was given approved by @yashdusing and hence the student is working on this new task. No worries haha

@Ram81 Ram81 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@Jatana Nice work, LGTM 👍

@Ram81
Ram81 merged commit 2787716 into Cloud-CV:master Oct 29, 2018
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.

5 participants