-
Notifications
You must be signed in to change notification settings - Fork 235
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add SearchBar for layers selection #400
Conversation
Jatana
commented
Oct 27, 2018
There was a problem hiding this comment.
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.
elem.style.display = 'none'; | ||
} | ||
} | ||
for (let elem of $('.panel-heading')) { |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
ide/static/css/searchbar_style.css
Outdated
|
||
.matched-search-char { | ||
color: rgb(69, 80, 97); | ||
// text-decoration: underline; |
There was a problem hiding this comment.
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.
ide/static/css/searchbar_style.css
Outdated
background: none; | ||
border: none; | ||
color: rgb(69, 80, 97); | ||
// border-bottom: 1px solid black; |
There was a problem hiding this comment.
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.
ide/static/js/content.js
Outdated
@@ -1275,7 +1275,9 @@ class Content extends React.Component { | |||
/> | |||
<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"> |
There was a problem hiding this comment.
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
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 ! |
@yashdusing Nice catch. It happens every time there is only a single character in search. @Jatana can you fix the issue. |
@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! :) |
@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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Jatana Nice work, LGTM 👍