Skip to content
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

Merged
merged 8 commits into from
Oct 29, 2018
Merged

Conversation

Jatana
Copy link
Contributor

@Jatana Jatana commented Oct 27, 2018

ezgif-2-c00195b7c555

Copy link
Contributor

@thatbrguy thatbrguy left a 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')) {
Copy link
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
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
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.

Copy link
Member

@Ram81 Ram81 left a 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.


.matched-search-char {
color: rgb(69, 80, 97);
// text-decoration: underline;
Copy link
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.

background: none;
border: none;
color: rgb(69, 80, 97);
// border-bottom: 1px solid black;
Copy link
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.

@@ -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">
Copy link
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
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
Copy link
Member

Ram81 commented Oct 28, 2018

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

@gautamjajoo
Copy link
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
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

Copy link
Member

@Ram81 Ram81 left a 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 👍

@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