-
Notifications
You must be signed in to change notification settings - Fork 23
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
#52 More explicit placeholder text for search box #66
Conversation
Hi @pooja2299 Thanks for the PR, indeed the text you propose is better than what was initially there 🏅 . Note that adding an icon would also have been a good idea. I can only recommend that now you grab another "good first issue". Best wishes |
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.
Thank you so much @pooja2299! I just asked for two minor modifications to the code (see below)
@pooja2299 to resolve the conflict here, the easiest way would be to click on the "resolve conflict" button on this page. It should take you to a web editor where you can see the conflict as shown in the screenshot below: |
Conflict resolved!:grin: Thanks @hmenager for such detailed guidance. |
Should we merge this now @bryan-brancotte & @hmenager ? |
@bryan-brancotte will not be much available today, so since I discussed this PR yesterday with him, I won't request a formal review from him today :) |
Added my solution to issue #52.