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

Adds search on mobile header [#1774] #1775

Merged
merged 6 commits into from
Nov 30, 2020
Merged

Adds search on mobile header [#1774] #1775

merged 6 commits into from
Nov 30, 2020

Conversation

ryancreatescopy
Copy link
Contributor

@ryancreatescopy ryancreatescopy commented Nov 11, 2020

Description

  • Added search icon to header.
  • Changes animation of drawer (enters from left)
  • Tweaked alignment of close icon on mobile search and mobile search from nav

Todo

  • Add modal background when pressing the search icon
  • Ensure icons have aria-labels
  • Ensure a user can tab to icons simply via keyboard

Related Issue

#1774

@ryancreatescopy
Copy link
Contributor Author

Realised, I've removed the X so you can't cancel the mobile menu easily. Need to fix.

@ryancreatescopy ryancreatescopy marked this pull request as ready for review November 20, 2020 15:14
Copy link
Contributor

@samajammin samajammin left a comment

Choose a reason for hiding this comment

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

Awesome! Love the idea to make search easier to access.

The UI functionality looks great but I don't like the overall duplication of code (having the SearchContainer & it's logic/children in both Nav/index.js & Nav/mobile.js). We should refactor this so it all lives in one place. Happy to pair on this next week / do it myself when there's time available.

@ryancreatescopy
Copy link
Contributor Author

Awesome! Love the idea to make search easier to access.

The UI functionality looks great but I don't like the overall duplication of code (having the SearchContainer & it's logic/children in both Nav/index.js & Nav/mobile.js). We should refactor this so it all lives in one place. Happy to pair on this next week / do it myself when there's time available.

For sure. I had the same thought that it should live separately in one place and we reference it from index and mobile.

@samajammin
Copy link
Contributor

@ryancreatescopy - refactored this so that:

  • State is maintained by the parent Search/Index.js component
  • Mobile search UI is consolidated into the Search/Mobile.js component

Please double-check the functionality is what's expected but I believe this achieves our goal.

samajammin
samajammin previously approved these changes Nov 23, 2020
@ryancreatescopy
Copy link
Contributor Author

@ryancreatescopy - refactored this so that:

  • State is maintained by the parent Search/Index.js component
  • Mobile search UI is consolidated into the Search/Mobile.js component

Please double-check the functionality is what's expected but I believe this achieves our goal.

Awesome! This pretty much does everything I intended! Couple things:

  1. Looks as though the changes have shifted the Search bar, dark mode icon, and Languages navItem to the left on desktop. Compare it with prod to see a small indent.
  2. When you press on the mobile search in the nav, it loads the menu first and then the search drawer slides over it. Could we do it without first loading the menu drawer?

Copy link
Member

@wackerow wackerow left a comment

Choose a reason for hiding this comment

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

These two changes will allow the Search menu to open independently of the menu (eliminating that animation glitch).

Only small issue here relates to PR #1960 which I posted recently, related to the z-index of the BottomMenu. I overlooked that this was still in the works when I posted that... If you pull the <SearchContainer> out of the <MenuContainer> then a z-index on the BottomMenu would not work to force it on top.

That being said, with two dedicated buttons in the nav bar, both for search and for the menu, it may not be worth bothering to have the bar at the bottom while searching as I had previously suggested in #1959, and just keep that exclusively in the menu.

src/components/Nav/Mobile.js Outdated Show resolved Hide resolved
src/components/Nav/Mobile.js Outdated Show resolved Hide resolved
src/components/Nav/Mobile.js Show resolved Hide resolved
@samajammin samajammin dismissed stale reviews from ghost and themself via 29f0e21 November 30, 2020 20:26
Copy link
Contributor

@samajammin samajammin left a comment

Choose a reason for hiding this comment

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

Made the edits you suggested @ryancreatescopy @wackerow - merging in!

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.

None yet

3 participants