-
Notifications
You must be signed in to change notification settings - Fork 3
Mobile Search Improvement #115
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
base: main
Are you sure you want to change the base?
Conversation
EMaksy
left a comment
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.
Hey @jagabomb thanks for the contribution, it looks good, just left a comment on the script.
| {{/if}} | ||
| <script> | ||
| function toggleSearch() { | ||
| var searchField = document.getElementById('search-wrapper'); |
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.
Try to use let or const instead of var overall ,as modern js recommends using let and const because they are block-scope
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/let
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.
Or we use es6 syntax?
const toggleSearch = () => {
document.getElementById('search-wrapper')?.classList.toggle('hidden');
};
check out the ternary operator
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Conditional_operator
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.
So the lint command doesn't like the use of let for elements. Also I could not get the const toggleSearch = () => {..} to work. Anyway, I have reworked this to use 05-mobile-navbar.js file so that I could make the Menu close when the Search button is clicked and vice versa.
1ff145e to
cbf6b10
Compare
Adds some design improvements to the usability of the search for mobile layouts mostly. The search tool can be accessed by using the search icon button which then display the search field. The close/clear button (X) helps to close the search for now. There is also a minor font change for the desktop search field font size. EOS icons have also included as a CDN import so that we can update other icons in the future to make it more closer to the overall Trento design.
Existing Mobile Search:

Mobile Search with Search Icon Button:


Proposed Behaviour between Search and Menu Panel States
