improve search #535
improve search #535
Conversation
|
Code Climate has analyzed commit b99d41f and detected 0 issues on this pull request. View more on Code Climate. |
|
@jywarren @sashadev-sky @SidharthBansal @gauravano @publiclab/is-reviewers please review |
|
@avsingh999 this issue already has a PR open for it at #511 , so I am going to close it. Please contribute there by providing a review if you have additional thoughts on it! Thank you and sorry to close this one on you |
|
@avsingh999 I misspoke! I see you are not working on the same issue as @divyabaid16. Reviewing now |
| @@ -22,6 +22,26 @@ | |||
| <%= render 'layouts/footer' %> | |||
|
|
|||
| <script> | |||
| $("input#search-input").on({ | |||
| keydown: function(e) { | |||
| if (e.which === 32) | |||
There was a problem hiding this comment.
the which attributes has been deprecated in favor of the key attribute. It's now compatible across all browsers (see here) so lets start using it? I think the replacement would be e.key === " " but please double check!
There was a problem hiding this comment.
@avsingh999 I requested some changes! Let me know your thoughts.
Also can you please open a new issue for this and reference it instead of the current one you're referencing. It makes it a lot easier to review and people are less likely to duplicate issues in this case
@sashadev-sky Sorry I had added wrong issue number. #520 is correct.
thank you
| @@ -22,6 +22,26 @@ | |||
| <%= render 'layouts/footer' %> | |||
|
|
|||
| <script> | |||
| $("input#search-input").on({ | |||
There was a problem hiding this comment.
I think this is better moved into a separate file rather than application.html.erb, to stick with modularity
There was a problem hiding this comment.
@sashadev-sky I also think But I don't find any file to add this,So I was thinking to add all script in new .js file.
thank you : )
|
@avsingh999 I requested some changes! Let me know your thoughts. Also can you please open a new issue for this and reference it instead of the current one you're referencing. It makes it a lot easier to review and people are less likely to duplicate issues in this case |
@sashadev-sky Sorry for this i have added wrong issue number.#520 is correct |
|
@avsingh999 No problem! I like the plan to move it into a new js file. Mention me for another review when you're ready 👍 |
|
@avsingh999 im going to close this because I think it's been abandoned and the implementation wasn't up to a point where it was working. Please feel free to reopen if you decide to come back to it! Note that some changes were made towards this since you opened this issue in #630 |
Fixes #520 (<=== Add issue number here)
Make sure these boxes are checked before your pull request (PR) is ready to be reviewed and merged. Thanks!
rake test@publiclab/reviewersfor help, in a comment belowIf tests do fail, click on the red
Xto learn why by reading the logs.Please be sure you've reviewed our contribution guidelines at https://publiclab.org/contributing-to-public-lab-software
Thanks!