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: Add search button that looks like field #1233

Merged
merged 9 commits into from Jun 30, 2023

Conversation

choldgraf
Copy link
Collaborator

@choldgraf choldgraf commented Mar 5, 2023

I have gotten some feedback that the search button in the top right is a little bit harder for people to find and notice. In addition because it's just the single icon, some folks have reported they assumed it was just to trigger "in-page search" in the browser rather than docs-wide search.

I noticed that in many other docs sites, they have a "trigger search button" that still looks like a field. Would it be useful if we made this an option in our theme? Either by a component people could add, or a flag to trigger "behave like a field" in configuration, or a CSS rule on our current search button that would make it "expand" at wide screen widths and shrink to just the icon at short widths? (I think for mobile view we still want the button behavior, this is only for the wide screen view)

To prototype this I put together this little PR. The behavior is the exact same, but the "search button field-style" is just more noticeable. What do people think? Here's a little GIF of what it looks like in comparison

ShareX_NmIUKme5cE

@drammock
Copy link
Collaborator

this LGTM, I especially like that the keyboard shortcut is visible before user interacts with it. Based on your GIF it looks like having both the icon-style button and field-style button on the same page works fine, which means there's no risk of controlling it via "include this template name in the relevant part of your conf.py" (because there's no risk of breakage if people include both by accident).

One request: needs to be documented in our user guide.

One nitpick suggestion: the fully-rounded corners of the search field look visually out of sync with the "only slightly rounded" rectangles elsewhere in our theme.

@trallard
Copy link
Collaborator

trallard commented Mar 22, 2023

As a note and from an accessibility and usability point of view it's always best to have the icon + text of what it means (because symbols are not universal).
Alternatively having tooltips also helps - but for this search I would prefer replacing with the explicit icon + search text version.

There are studies revealing that from a usability pov user experience in order of best to worse are:

  1. Text only
  2. Icon + text
  3. Icon + tooltip
  4. Icon

@choldgraf
Copy link
Collaborator Author

Hey all - I have limited time to do github stuff but can try to do a quick iteration so that this is ready to go.

I'll add this to the docs and will fix the rounded corners you note @drammock

@trallard I couldn't figure out if you were making a specific recommendation for this PR or not. Given the info in your comment, do you think this PR should be different? Are you suggesting the default should be this larger "text + button" combo rather than just the icon?

@trallard
Copy link
Collaborator

Sorry was writing my comment in a rush as I am meant to be on holidays lol

But yes I recommend making the default the "icon + text" variation

@drammock
Copy link
Collaborator

I recommend making the default the "icon + text" variation

+1 on this. I think ideally there would be an option to use the new field-like button on big screens and automatically switch to just the icon on small screens, but IDK how easy that will be and it would make sense as its own PR.

@12rambau
Copy link
Collaborator

AS on small screen, all the titles are disapearing I think there is plenty of room for the field-search button, don't you think ? In any case I agree it looks better in every way so it should be set as default

@drammock
Copy link
Collaborator

on small screen, all the titles are disapearing I think there is plenty of room for the field-search button, don't you think ?

if that's the case then awesome, no need for the separate PR to auto-switch to icon on small screen. I just haven't tested it so I don't know.

@choldgraf
Copy link
Collaborator Author

choldgraf commented Mar 27, 2023

Would you all be +1 on the following proposal?

  • Update our current search button behavior so that:

  • = mid sized screens, it looks like a search field, with Search... inside. Like so:

    image

  • < mid-sized screens, it collapses down to a single button with a circular style? Like so:

    image

This mimics how docusaurus does their search field button.

@trallard
Copy link
Collaborator

I am +1 - there are some inconsistencies now with icons (and interactive states across the theme), but those are some things I will be tackling separately as part of a11y work so I am good with this proposal

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.

could you make it the default search in the theme.conf ?

@12rambau
Copy link
Collaborator

@trallard, @drammock, I made it the default in the last commit and it feels a bit too big. Is it me or do you agree ?

@choldgraf
Copy link
Collaborator Author

Definitely agree. I feel like it should be the same height as the version dropdown menu.

(and thanks for the push, I am super limited in my bandwidth right now so please make whatever changes you like)

@drammock
Copy link
Collaborator

agreed, it looks too big.

@12rambau
Copy link
Collaborator

I fixed the size on my local computer but the build is failing on all 3.11 tests and the RDT one (3.9) when executing nodeenv.py. Before I started loosing hair on this one does anyone fluent in reading nodeenv error traceback ?

@drammock
Copy link
Collaborator

I fixed the size on my local computer but the build is failing on all 3.11 tests and the RDT one (3.9) when executing nodeenv.py. Before I started loosing hair on this one does anyone fluent in reading nodeenv error traceback ?

looks like the nodeenv package fails when trying to copy a prebuilt node environment, because the source folder is not found (glob.glob(src_folder_tpl) returns an empty list, which then cannot be successfully unpacked). I have no idea why.

I don't understand why package-lock.json is changing in this PR since package.json is not changing. First thing I would try is to restore package-lock.json to whatever it is on main and see if this problem goes away 🙈

@12rambau
Copy link
Collaborator

O don't understand, I copy pasted the package.json and package-lock.json from main and they are still marked as "changed" in the PR status. Can someone compared the files and confirm that I'm not getting crazy ?

@choldgraf
Copy link
Collaborator Author

Hmmm this section seems relevant:

      [webpack-cli] Failed to load '/home/docs/checkouts/readthedocs.org/user_builds/pydata-sphinx-theme/checkouts/1233/webpack.config.js' config
      [webpack-cli] Error: Cannot find module 'optimize-css-assets-webpack-plugin'

Maybe because we're removing this dependency here:

https://github.com/pydata/pydata-sphinx-theme/pull/1233/files#diff-7ae45ad102eab3b6d7e7896acd08c427a9b25b346470d7bc6507b6481575d519L17

But it's required elsewhere?

Full error:

      [webpack-cli] Failed to load '/home/docs/checkouts/readthedocs.org/user_builds/pydata-sphinx-theme/checkouts/1233/webpack.config.js' config
      [webpack-cli] Error: Cannot find module 'optimize-css-assets-webpack-plugin'
      Require stack:
      - /home/docs/checkouts/readthedocs.org/user_builds/pydata-sphinx-theme/checkouts/1233/webpack.config.js
      - /home/docs/checkouts/readthedocs.org/user_builds/pydata-sphinx-theme/checkouts/1233/node_modules/webpack-cli/lib/webpack-cli.js
      - /home/docs/checkouts/readthedocs.org/user_builds/pydata-sphinx-theme/checkouts/1233/node_modules/webpack-cli/lib/bootstrap.js
      - /home/docs/checkouts/readthedocs.org/user_builds/pydata-sphinx-theme/checkouts/1233/node_modules/webpack-cli/bin/cli.js
      - /home/docs/checkouts/readthedocs.org/user_builds/pydata-sphinx-theme/checkouts/1233/node_modules/webpack/bin/webpack.js
          at Function.Module._resolveFilename (node:internal/modules/cjs/loader:933:15)
          at Function.Module._load (node:internal/modules/cjs/loader:778:27)
          at Module.require (node:internal/modules/cjs/loader:1005:19)
          at require (node:internal/modules/cjs/helpers:102:18)
          at Object.<anonymous> (/home/docs/checkouts/readthedocs.org/user_builds/pydata-sphinx-theme/checkouts/1233/webpack.config.js:16:33)
          at Module._compile (node:internal/modules/cjs/loader:1101:14)
          at Object.Module._extensions..js (node:internal/modules/cjs/loader:1153:10)
          at Module.load (node:internal/modules/cjs/loader:981:32)
          at Function.Module._load (node:internal/modules/cjs/loader:822:12)
          at Module.require (node:internal/modules/cjs/loader:1005:19) {
        code: 'MODULE_NOT_FOUND',
        requireStack: [
          '/home/docs/checkouts/readthedocs.org/user_builds/pydata-sphinx-theme/checkouts/1233/webpack.config.js',
          '/home/docs/checkouts/readthedocs.org/user_builds/pydata-sphinx-theme/checkouts/1233/node_modules/webpack-cli/lib/webpack-cli.js',
          '/home/docs/checkouts/readthedocs.org/user_builds/pydata-sphinx-theme/checkouts/1233/node_modules/webpack-cli/lib/bootstrap.js',
          '/home/docs/checkouts/readthedocs.org/user_builds/pydata-sphinx-theme/checkouts/1233/node_modules/webpack-cli/bin/cli.js',
          '/home/docs/checkouts/readthedocs.org/user_builds/pydata-sphinx-theme/checkouts/1233/node_modules/webpack/bin/webpack.js'
        ]
      }
      error: js-build-failed

@drammock
Copy link
Collaborator

drammock commented Apr 21, 2023

O don't understand, I copy pasted the package.json and package-lock.json from main and they are still marked as "changed" in the PR status. Can someone compared the files and confirm that I'm not getting crazy ?

Can you try

git fetch upstream
git switch main
git merge --ff-only upstream/main
git switch search-field-button
git restore --source main -- package.json
git restore --source main -- package-lock.json

@drammock
Copy link
Collaborator

actually never mind my last comment; it looks like the branch needs a rebase? The change to package.json is adding axe-core which is already in main. So to fix you can either

  1. restore package.json and package-lock.json to their state in b8a2d76, or
  2. rebase and then do the commands in my previous comment

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.

dependency problem is now solved

@choldgraf
Copy link
Collaborator Author

I don't think this is behaving how we want on mobile screens. It is.being cut off and doesn't seem to be responding to screen size. Here is what the button looks like in my phone:

Screenshot_20230522-083202.png

@choldgraf
Copy link
Collaborator Author

choldgraf commented Jun 30, 2023

I had a moment, so pushed a quick fix to the CSS so that this is now responsive. Are folks OK on merging this and iterating? I think I was the one that blocked this before and I'd be happy to move forward as long as there are no objections.

@drammock drammock merged commit eb8bdee into pydata:main Jun 30, 2023
16 checks passed
@drammock
Copy link
Collaborator

thanks for unblocking this @choldgraf!

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

4 participants