Skip to content
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: mobile site search for navbar #1559 #5

Merged
merged 17 commits into from
Nov 20, 2024
Merged

Conversation

mpleroux
Copy link
Member

@mpleroux mpleroux commented Nov 13, 2024

πŸ”— Linked issue

CED-1950: Implement mobile site search navbar

❓ Type of change

  • πŸ“– Documentation (updates to the documentation or readme)
  • 🐞 Bug fix (a non-breaking change that fixes an issue)
  • πŸ‘Œ Enhancement (improving an existing functionality like performance)
  • ✨ New feature (a non-breaking change that adds functionality)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

πŸ“š Description

  • Refactored mobile navbar to accommodate new search button (magnifying glass)
  • Restyled mobile search form to match design in Figma
  • Added an ID prop to EsSearchBar to prevent duplicate IDs from multiple forms on the same page
  • Refactored EsSearchBar to accommodate mobile version without breaking current desktop version
  • Restyled mobile search form
  • Prevented overlay element from covering mobile search form
  • Added new ID prop to EsSearchBar to prevent duplicate form IDs
  • Adjusted mobile navbar markup to use showSearch prop
  • Handle searchForm.focus() for either mobile or desktop
  • Hide mobile search form when mobile menus are opened
  • Hide desktop search form when desktop menus are opened

To Do

  • Hide search form when mobile menus are opened
    • Attempts to resolve this have resulted in circular logic
  • Close mobile menus when opening mobile search form
    • See comments below for details

πŸ₯Ό Testing

🧐 Feedback Requested / Focus Areas

  • It may not be possible to use one nav-search-bar element for both mobile and desktop due to their locations in the navbar DOM

πŸ“ Checklist

  • I have linked an issue or discussion.
  • I have updated the documentation accordingly.
  • I have documented testing approach

@mpleroux mpleroux marked this pull request as ready for review November 13, 2024 21:33
@mpleroux mpleroux requested a review from hroth1994 November 13, 2024 21:33
@hroth1994
Copy link

πŸ‘€

Copy link
Collaborator

@ericdouglaspratt ericdouglaspratt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see some CI failures on linting

@mpleroux
Copy link
Member Author

mpleroux commented Nov 18, 2024

I see some CI failures on linting

Thanks. I haven't been able to determine the cause of that error or other adjustments made to lines of code that I didn't modify. I believe Prettier is to blame, but the local editor configuration seems fine.

I disabled the VSCode extension in my workspace for that repo and manually reverted the changes. CI checks are now passing.

Copy link
Collaborator

@ericdouglaspratt ericdouglaspratt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the circular logic when you call toggle_search_bar from show_mobile_menus?

es-vue-base/src/lib-components/EsNavBar.vue Outdated Show resolved Hide resolved
Copy link
Collaborator

@ericdouglaspratt ericdouglaspratt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Understood that we have a small edge case where wide mobile can open a menu and then the search bar, but for an A/B test, I don't think that's serious enough to warrant spending more time on.

@mpleroux mpleroux merged commit 1a49270 into main Nov 20, 2024
1 check passed
@mpleroux mpleroux deleted the CED-1950-mobile-navbar branch November 20, 2024 19:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants