carpentries / workbench

Repository for Discussions and Materials about The Carpentries Workbench
https://carpentries.github.io/workbench/
Creative Commons Attribution 4.0 International
17 stars 7 forks source link

Search functiontionality is disabled (text box not selectable) #8

Closed anenadic closed 5 months ago

anenadic commented 2 years ago

The search box at the top only provides for search on the actual page and not search across the whole lesson site. Is this the intended behaviour?

zkamvar commented 2 years ago

Hi @anenadic,

Thank you for bringing this up! I've transferred this to the workbench repository since this particular issue is for the workbench as a whole as opposed to the documentation.

Short answer

The search functionality is currently disabled and I apologise for not flagging it as disabled. I am working on implementing it in an accessible manner, but it may take some time.

Long answer

The search bar exists because we asked for it during the design of the frontend. The designer added the search bar with a button and the frontend contractors placed it in the lesson, but without functionality behind it because how exactly search results would be displayed was unclear.

That being said, our infrastructure is built on top of {pkgdown} and search functionality does exist in {pkgdown}, as you can see from the {sandpaper} package website, but we do not import any of their JavaScript or CSS the structure of the website is so different.

I thought I opened an issue to add functionality to the search button, but it turns out that I only generated a commit that begins the process with a note about what I found:

While Bootstrap 5 may have search enabled through their forms, it turns out that it's difficult for me to find documentation for this specific task. Moreover, the pkgdown searchbar never had a button associated with the search... and for good reason: the search is dynamic; it does not bring you to a page with your search results; it gives you the search results as a dropdown menu, so you go to your result by clicking "enter". It makes sense! I think we can provide a blank page for search results, but we will need to test out the accessibility for sure.

The steps for me to resolve this are:

zkamvar commented 2 years ago

Note about the search functionality, I think the appropriate path is to take the approach of Mozilla Developer N(etwork)? and implement the search field that does two things:

  1. creates a dynamic dropdown search as you type
  2. appends a "didn't find what you were looking for" item at the end to link to a search page
  3. button click sends to a search page

The search page they have looks like this: https://developer.mozilla.org/en-US/search?q=localStorage

elichad commented 1 year ago

@zkamvar I just ran into this as well. Do you plan to work on this soon? If not, it might be better to hide the search box in the meantime to reduce confusion.

martinosorb commented 1 year ago

Is there currently another smart way of searching within a whole lesson? This is useful for maintainers. Cheers

zkamvar commented 1 year ago

At the moment, a good way to search the whole lesson is to visit the all in one page, which is located at the bottom of the schedule: https://carpentries.github.io/workbench/transition-guide.html#aio-instructor

The page is always called "aio.html", so you can visit there directly by adding /aio.html to the URL. For example , this is the all in one page for the workbench tutorial: https://carpentries.github.io/sandpaper-docs/aio.html

zkamvar commented 1 year ago

And I apologise on my delay with this issue. There are many things that have come up since it was opened -_-

martinosorb commented 1 year ago

No worries, I'm aware you have a lot of things to think about. The one-page version works perfectly, thank you.

kekoziar commented 1 year ago

The search box is not selectable. I've tried on multiple SWC lessons, using two different browsers.

zkamvar commented 1 year ago

@kekoziar, it has not yet been enabled. See https://github.com/carpentries/workbench/issues/8#issuecomment-1150046891 for details.

I have gotten part of the way with https://github.com/carpentries/varnish/pull/86, which you can see the result in my fork of instructor training (https://zkamvar.github.io/instructor-training/), but it's not quite ready as there is still a bit of accessibility testing that needs to happen.

kekoziar commented 1 year ago

Got it. Thanks. I saw the initial "The search box at the top only provides for search on the actual page and not search across the whole lesson site," and saw that it wasn't enabled, but missed that the box is not selectable. I thought it was only searching partially.

Do you think you can edit your initial answer to say "The search functionality is currently disabled (the search box is not selectable)"?

ostephens commented 9 months ago

Given this is a long outstanding issue I wonder if it is possible/worth while to make it much clearer that search doesn't work, or even remove the search bar completely from the display? It isn't at all obvious to me that the search is disabled - I tried clicking many times, checked across browsers, before realising that the field was disabled by checking the code.

I think it would be a better solution to remove the search box until it works. But if this is not possible perhaps it needs to be more obvious it is not implemented yet - by some onscreen text maybe?

froggleston commented 9 months ago

Hi @ostephens - the transition to the Workbench has been a lengthy process and this has been on the wayside for a while, but @zkamvar has been making progress in the linked draft PR: https://github.com/carpentries/varnish/pull/86. We'd like to try and get this done by the end of the year, but as we have a lot of other higher priority work, this might not happen. We might be able to fit in a hotfix to more clearly render the search box as disabled, perhaps with a "Disabled" placeholder or some such, but again, I'd have to catch up with Zhian to see if this makes sense given his own thoughts about progress with the draft PR.

kekoziar commented 9 months ago

How about Select "See all in one page" from menu to search lesson? It's wordy, but perhaps not everyone knows that page exists, where they can ctrl+f to search at least the lesson.

ErinBecker commented 6 months ago

@froggleston and I have made some updates to Zhian's PR for introducing search functionality and are have reached out to Maintainers to test. The Instructions for Testing Search Bar in Workbench document includes instructions for testing locally and on GitHub. We are asking folks to provide feedback either to this issue or in response to this email on the Maintainers TopicBox by 13 March.

quist00 commented 6 months ago

Thanks for tackling this important feature. Here are some of my impressions.

  1. The magnify glass icon can float around quite a bit and go outside the search box when full screen at 2560 X 1440.
  2. The cancel icon can overlap with the magnify icon depending on where it is floating.
  3. The magnify glass icon acts as a button, but not sure it should. It reloads page, blanks out search box, but leaves search in querystring
  4. Maybe magnify button could take you to full page of results that you could browse forward and back from as browse for what you are looking for
  5. does not seem to AND or OR search terms. Just searches for exact strings. Thus something like "plot pandas" or "plot bokeh" return 0 results.
  6. Lots of results are returned but it is difficult to browse them if you are not sure what is the one you are really looking for.
  7. Maybe when you hover over search it could drop down with your last search results rather than having to re-enter search to try another result

Edited by @ErinBecker to add numbers for easy reference in discussion below.

astroDimitrios commented 6 months ago

I've just had a go at testing this locally and on this fork.

  1. It doesn't for me search text / paragraphs. The image below shows me trying to search for the word Parallel (which you can see in the first few lines of text at the bottom) but it finds nothing. image

  2. When I do get a match it shows me almost duplicate results for the instructor view vs the learner view vs all in one but doesn't seem to highlight the part I searched for. image

  3. If I search for the word Fortran sometimes it lists the code-block title and often when I click on the magnifying glass for this search it will send me straight to a 404 page.

ErinBecker commented 6 months ago

Thank you @quist00 and @astroDimitrios for going through the testing process and providing detailed feedback. This is really helpful.

I have some inkling of how to go about solving issues 1, 2, and 3 raised by @astroDimitrios and possibly issue 5 raised by @quist00. Issues 1-4 and 6-7 raised by @quist00 are pretty far outside of my comfort zone and into some detailed CSS manipulation.

@froggleston - I'm going to see what I can do about addressing @astroDimitrios's 1,2,3 and @quist00's 5 and will post updates here.

ndporter commented 6 months ago

Just tested locally (on the instructor training). In addition to what @astroDimitrios brought up:

  1. it would be nice to have some way to see results with snippets before opening the links if you know the text but not the header you want. I imagine this as an option at the bottom (or if you press enter) that opens a more traditional search results page. This may also help accessibility for search with screen readers, since going to each result and waiting for it to read aloud until you reach the relevant sentence would be frustrating (though I appreciate that the cursor is moved to the section, not top of page).
  2. The results returned should probably be limited to the selected version (learner or instructor) rather than doubling most results. You could make a case for including pages that are only on one version to both sets of results but it seems to go against the idea of different versions.
  3. Likewise, the triplicate of including the all-in-one view seems even more unnecessary if it's avoidable.
  4. Could we change the default behavior to be open in new tab unless there's a reason not to? To me, moving focus is ok but I'd rather not leave the current page most of the time.
  5. Not critique but suggestion: we should make sure that maintainers know that having clear yaml episode and md fenced div titles will become even more important.

I did find one other glitch: when results are in the instructor notes page (at least on the instructor training), I get null in the path (see image below). Relatedly, the learner version seems to populate even for pages in the instructor folder (like the instructor notes. Screenshot 2024-03-11 at 14 27 07

ErinBecker commented 5 months ago

Thank you @ndporter @astroDimitrios and @quist00 for taking the time to test out this feature and provide detailed feedback. It's really great to see members of the community engaging with the Workbench and helping to make it better!

After banging my head against these issues for a while, it seems to me that using the Algolia Docsearch package to implement search on our lessons (the strategy used in https://github.com/carpentries/varnish/pull/86 and https://github.com/carpentries/sandpaper/pull/495 and to which the feedback above is directed), isn't a good out-of-the-box solution for us, as it doesn't give us the desired look and feel. Our ability to customize Docsearch is also very limited. Pkgdown does also provide a built-in search capability, but according to the docs, "the only available customisation is excluding some paths from the search index." Another option we've considered is embedding a Google search widget, but the time it takes Google to re-index a page depends on a lot of factors, and it may be weeks before changes to the lesson show up in searches.

Given these limitations, we're going to move forward with @kekoziar's suggestion of making the option to search the all in one page more visible. My next update here will include a link to a set of PRs (in progress) for implementing this change.

varnish PR: https://github.com/carpentries/varnish/pull/131 sandpaper PR: TBA

froggleston commented 5 months ago

Thanks to @ErinBecker 's awesome efforts, the problematic search box has been replaced by a button that will take you to the All in One page that will facilitate easier searching by the browser's built-in search feature. There were simply too many issues with the Algolia solution that would have taken much longer to fix, so we felt this was the next best thing. Thanks @ErinBecker !