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

ENH: Make search field an overlay with button click #744

Merged
merged 14 commits into from
Jun 23, 2022

Conversation

choldgraf
Copy link
Collaborator

@choldgraf choldgraf commented Jun 21, 2022

This is a proof of concept to explore the UX around using an icon for search instead of the sidebar "search field". Here are the major points here:

  • Removes the search field from the left sidebar
  • Only shows the left sidebar if there are components in there and pages to list
  • There's now a search icon in the top right
  • Clicking that icon opens a search field overlay just below, and places the cursor there
  • Clicking the icon again causes it to disappear
  • The landing page now has no left sidebar by default, since there are no pages to be shown and no more search component
  • Improves some of the search field etc colors

So the only different in UX from the current search workflow is that you click, wait a second for the new page to load, and then start typing, rather than typing from the page you used to be on before being redirected.

New behavior (ignore the # differences, I think those only show up on RTD):

chrome_unsuqux2C0

Old behavior:

chrome_z4O4LXKesD

Actions

  • Do others agree or disagree that this is an improvement on our current search UI/UX?
  • Make tests pass

closes #620 closes #434 closes #254 closes #560

@choldgraf choldgraf changed the title [WIP] Use search icon instead of sidebar field Make search field an overlay with button click Jun 21, 2022
@choldgraf
Copy link
Collaborator Author

choldgraf commented Jun 21, 2022

OK after taking the feedback in #620, this makes a few updates to make the search field show up as an overlay when you click that button, rather than directing you to the new page.

I've updated the top comment with the latest update. Would love to know what @drammock and @12rambau think. If folks are happy then I'll get the tests passing

Copy link
Collaborator

@12rambau 12rambau left a comment

Choose a reason for hiding this comment

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

I really like this implementation. some minor comments from my side:

  • It would feel more natural if the search field disappear when blur:

image

  • It behaves weirdly on small screen, maybe it could be centered and with a small padding:

image

@choldgraf
Copy link
Collaborator Author

choldgraf commented Jun 21, 2022

Thanks for these comments! Could you describe more what you mean by "if the search field disappear when blur"? I'm not sure I understand...

I wonder if it'd be better to just have the overlay centered and at ~80% width on both wide and mobile screens, maybe with a bigger blur in the background to differentiate from the page content? Is that what you were referring to?

@12rambau
Copy link
Collaborator

if the search field disappear when blur

If I search for something but someone realize I don't need it anymore, I would like to click outside the searche field ("blur" event as it's automatically focused) and see it disapear. In the current implementation I'm force to click on the mignifying glass again

@12rambau
Copy link
Collaborator

I wonder if it'd be better to just have the overlay centered and at ~80% width on both wide and mobile screens, maybe with a bigger blur in the background to differentiate from the page content? Is that what you were referring to?

exactly

@choldgraf
Copy link
Collaborator Author

OK I think this one makes that possible. I've added a little "overlay div" behind the main search field and connected the same JS event to it, so clicking outside the search field will now make it hide. Also made it bigger and centered it so it behaves the same on mobile.

Check it out:

chrome_oTCcXv08oo

@drammock
Copy link
Collaborator

looks pretty nice! Some comments:

  • the magifying glass is missing from the search results page. Functionally this is ok (there's a search box at the top-center of the content) but visually it makes the position of top bar elements change. Also I think there's a small functional advantage to keeping it there: if you've scrolled down through a long list of search results and not found what you need, having the magnifying glass available means you needn't scroll back up to the search box to do a different search
  • search is a site-internal function, it seems semantically out of place put next to all the icons that (generally) take you to a different site (twitter, github, etc). It's more similar to the dark/light toggle in that regard, so maybe it should go at the beginning of the topbar icons?
  • might like to keep the old-style search box as an option? mostly I'm worried about lots of users with ingrained habits wondering "where did the search box go?"

@choldgraf
Copy link
Collaborator Author

Yeah i agree re: the magnifying class being gone on the search page. The problem was that Sphinx' search script seems to look for only one search field on the have to populate it with the search text. So we have a choice between "have the search text pre-populated in the field" and "show the magnifying glass on the search page".

Hmm maybe we could make the magnifying glass just jump to the top and activate the in-page search bar only when it's on that page?

@drammock
Copy link
Collaborator

maybe we could make the magnifying glass just jump to the top and activate the in-page search bar only when it's on that page?

SGTM!

@12rambau
Copy link
Collaborator

could you add the cmd+k key binding to the search field ? (it's available in many site and avoid to look for the search field on the page)

@choldgraf
Copy link
Collaborator Author

choldgraf commented Jun 21, 2022

OK all comments addressed! The latest push does the following:

  • Moves the search bar to the left of the navbar end items by default (I agree that's a more natural place)
  • Fixes up some spacing CSS on the navbar items in general
  • Adds a CTRL + K keyboard shortcut to activate the search
  • Makes the search button show up on search.html and has it jump to the search field.
  • Fixes tests

Could @12rambau test that CMD+K works as well? I'm on a PC so can't test!

@drammock
Copy link
Collaborator

ctrl+k works on linux, on regular pages, and also on the search results page! nice.

@12rambau
Copy link
Collaborator

12rambau commented Jun 21, 2022

cmd+k doesn't work on Mac. I was forced to use ctr+k. you should have a look to this: https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent/metaKey

@choldgraf
Copy link
Collaborator Author

choldgraf commented Jun 22, 2022

Fixed! The latest push does:

  • Adds cmd support
  • Adds some <kbd> elements to the right of the search UI to hint at the shortcuts
  • Updates our <kbd> styling to be similar to what GitHub uses (slightly off background w/ a border)
  • Adds a max-width to the search form popup so it isn't super long on wide screens

Here's a demo:

chrome_XnjMHMvw6j

@12rambau
Copy link
Collaborator

The rendering is awesome and the macOS shortcut works nicely

One last comment:
could you make the content of the first kbd responsive to OS ? "cmd or "⌘" for mac and "ctrl" for all the others.

@choldgraf
Copy link
Collaborator Author

choldgraf commented Jun 22, 2022

OK I think that this should now work - @12rambau can you try again with a Mac? I also cleaned up the HTML implementation a bit because I realized we had an unnecessary conditional in there.

@12rambau
Copy link
Collaborator

awesome !
LGTM !

@12rambau
Copy link
Collaborator

This is also solving #254 so I added it in the PR message

@choldgraf
Copy link
Collaborator Author

OK I'll get the merge conflict resolved and then merge this one in!

@choldgraf choldgraf changed the title Make search field an overlay with button click ENH: Make search field an overlay with button click Jun 23, 2022
@choldgraf choldgraf merged commit bbc855b into pydata:main Jun 23, 2022
@choldgraf choldgraf deleted the enh-header-search-icon branch June 23, 2022 09:47
@mwaskom
Copy link

mwaskom commented Jun 25, 2022

This is really slick!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants