-
Notifications
You must be signed in to change notification settings - Fork 209
Conversation
@SanketDG @sakshi1499 @sammy1997 I have rebased the branch. |
systers_portal/templates/community/snippets/search-snippet.html
Outdated
Show resolved
Hide resolved
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.
LGTM apart from what @sammy1997 said about separating the js code, Before I approve this, I am going to test this end to end of the whole story taking into account the last 2 PRs.
@anitab-org/portal-maintainers when merging, please update the PR title to reflect our new Commit Style Guidelines. |
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.
I did run it locally, works fine for me. I agree with what sammy said , its always good to separate js and not have it in the html file. So please do change it and do follow the commit guidelines.
@SanketDG @sammy1997 API_KEY variable added. Please review it again :) |
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.
First set of reviews!
UX question for everyone: Right now, its pretty hard to see the search results. I think there is a scope for improvement here. Maybe we make the map a tad bit smaller? maybe add a loader to the ajax request for visual cue? Looking for ideas.
systers_portal/templates/community/snippets/search-snippet.html
Outdated
Show resolved
Hide resolved
systers_portal/templates/community/snippets/search-snippet.html
Outdated
Show resolved
Hide resolved
@SanketDG Changes have been made please have a look when you're free :) |
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.
LGTM!
Merging this PR. cc: @mayburgos @sakshi1499 @ritwickraj78 @SanketDG |
Sorry about that! Accidentally closed it! |
Description
Fixes #536
Type of Change:
Code/Quality Assurance Only
How Has This Been Tested?
Checklist:
Delete irrelevant options.
Code/Quality Assurance Only