-
Notifications
You must be signed in to change notification settings - Fork 14.7k
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
Fix search-icon when typing #44527
Fix search-icon when typing #44527
Conversation
Welcome @Subhajit009iitr! |
✅ Pull request preview available for checkingBuilt without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
/area web-development |
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.
Thanks @Subhajit009iitr
I've seen the approach and I have a few suggestions to make:
- Try to use an
fa search
(fontawesome search) icon instead of a background png; CSS would then change accordingly - The internal CSS is OK; Even better if you can accomodate it into
assets/custom.scss
OK I will make the changes today. |
@Gauravpadam I have updated the code using the |
layouts/partials/search-input.html
Outdated
> | ||
{{ else if .Site.Params.offlineSearch }} | ||
<div id="search-nav-container"> | ||
<head> |
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.
Why add this element?
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.
It was initially there in the code, I thought it may have some CSS properties attached to it
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.
Thanks
Please see my inline feedback for some further improvements on the approach
@Gauravpadam I updated all the changes as per your latest feedback. Please review it once |
Thanks, I'll be happy to lgtm if either:
|
I have moved the CSS component to |
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
Thanks for the PR @Subhajit009iitr!
LGTM label has been added. Git tree hash: 2de852f72799d384d946081b1f9baff637c00f7b
|
Some more web development issues that could be worked on - only if you'd want to. |
Ya I would Love to! I will look into them. Thanks |
Hello @Subhajit009iitr : Please could you add an appropriate description of the changes you've made in this PR and why you think these changes are necessary? This would help approvers understand your POV. Explicit hold on the PR till the description is added. |
I have updated the description as instructed here |
@divya-mohan0209 Is the description fine? |
Thank you @Subhajit009iitr for getting to this so promptly! |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: divya-mohan0209 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
This PR fixes the
Search-icon
disappearance issue when typing something in the search bar. Earlier the magnifying glass symbol was kept within the placeholder of the input form, so whenever something was typed theSearch-icon
would disappear too along with the placeholder commentsSearch this site
. I have fixed that in this PR and now the magnifying glassSearch-icon
stays in place at the left-most side of the search bar.This is in the context of the issue #44416.