-
Notifications
You must be signed in to change notification settings - Fork 1.8k
feat: Front Page: Move Search Function to Top Bar #6007
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
feat: Front Page: Move Search Function to Top Bar #6007
Conversation
|
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/eventyay/open-event-frontend/bazf13va4 |
Codecov Report
@@ Coverage Diff @@
## development #6007 +/- ##
============================================
Coverage 23.45% 23.46%
============================================
Files 511 512 +1
Lines 5478 5490 +12
Branches 67 67
============================================
+ Hits 1285 1288 +3
- Misses 4176 4185 +9
Partials 17 17
Continue to review full report at Codecov.
|
|
Thank you. Please change the following:
|
|
@mariobehling Pls have a look |
|
Please use #fafafa as bg color. Make default size of search bar smaller and don't expand it too much. GitHub expands it to show suggestions, we are not doing that currently. |
|
And it should also display and work on mobile |
|
Great, however I think mobile view should also contain the search bar in NavBar, or people won't notice it |
|
Then pls let me know where should I implement that. Bcz no space is in navbar |
|
If there is no space, do what eventbrite does |
|
Yes, please implement it in the top menu bar. Thanks |
|
@iamareebjamal Pls have a look |
|
Deployment failed with the following error: |
|
Search icon still black. Can't click on cross till I type something |
cross icon is working for me |
|
@iamareebjamal I have made the changes and cross icon is working |
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.
look nice to me.
only github actions / lint annotations needs to fix.
lint is not giving any error |
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.
Looks good 👍🏻
|
|
||
| @action | ||
| toggleSearchBar() { | ||
| document.querySelector('#mobile-bar').classList.toggle('show-bar'); |
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 not using ember for this? Merging this as it has been too long, but please create a follow up PR to change DOM manipulation to ember
|
Great job @daretobedifferent18 Please follow up with a PR with minor change I mentioned |
|
@iamareebjamal also should I do #6017 with it bcz I think it will be easy to do it together bring search bar there as well |
|
Yes |



Fixes #5927
Short description of what this resolves:
Move the search feature to the top bar and substitute "Browse Events" with "Search Events"
Changes proposed in this pull request:
Screenshot
Desktop

Checklist
developmentbranch.