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

Search #37

Merged
merged 27 commits into from
Jun 6, 2018
Merged

Search #37

merged 27 commits into from
Jun 6, 2018

Conversation

ralf57
Copy link
Collaborator

@ralf57 ralf57 commented Apr 5, 2018

No description provided.

@ralf57 ralf57 mentioned this pull request Apr 5, 2018
@ralf57 ralf57 changed the title WIP - Search Search Apr 18, 2018
@ralf57 ralf57 requested a review from jmuheim April 18, 2018 12:15
@jmuheim
Copy link
Collaborator

jmuheim commented Apr 18, 2018

Looking through the PR. Just in case you didn't notice: the branch has can't be merged at the time being.

 Conflicts:
	src/components/global/navbar/_navbar.scss
@jmuheim
Copy link
Collaborator

jmuheim commented Apr 18, 2018

The search input and submit button are focusable, even when visually hidden.

image

Please make sure they aren't, by using display: none or the hidden attribute.

@jmuheim
Copy link
Collaborator

jmuheim commented Apr 18, 2018

This "search" label should be below the <h2>.

image

<div class="search js-search">
<h2 class="visuallyhidden">Search</h2>
<button class="search--toggle">
<span class="visuallyhidden">Search</span>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Better call it Show / hide search.

Copy link
Collaborator

Choose a reason for hiding this comment

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

By the way, closing doesn't work yet. Is this easy to implement (by clicking on the icon again, or by pressing Esc)? Otherwise, naming the button "Open search" only makes more sense.

<h2 class="visuallyhidden">Search</h2>
<button class="search--toggle">
<span class="visuallyhidden">Search</span>
</button>
<input class="search--input" type="text">
Copy link
Collaborator

Choose a reason for hiding this comment

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

This input doesn't have an ID, so it can't have a <label>. But every input needs a label.

@@ -1,5 +1,8 @@
<div class="search">
<h2 class="visuallyhidden">Suche</h2>
<div class="search js-search">
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should add a role="search" here (which is one of the very few ARIA landmarks that don't have an HTML5 equivalent).

Copy link
Collaborator

@jmuheim jmuheim left a comment

Choose a reason for hiding this comment

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

Added some hints and comments.

@jmuheim
Copy link
Collaborator

jmuheim commented Apr 23, 2018

All in all, this looks very promising. 👍

At the time being though, it's sometimes unclear why certain results are shown upon the entered string:

image

In Google, besides the title of a search result, some extracts of the page are displayed, so the user has more context. Is something possible in SS360?

Other than that, examples are indexed, leading to useless and confusing suggestions (Hobbies):

image

Is it possible to remove certain pages from the index, for example */example.html?

@ralf57
Copy link
Collaborator Author

ralf57 commented Apr 24, 2018

@jmuheim I didn't do any configuration in SS360 yet, so the results are coming from the default setup and are expected. They will be much better after the fine-tuning.
If the content (and especially the page structure) is stable I will reserve some time to do it ASAP.

@jmuheim
Copy link
Collaborator

jmuheim commented May 16, 2018

It only displays results from 3 characters on. We either have to make this more clear, or display results already with 1 or 2 characters.

image
image
image

@jmuheim
Copy link
Collaborator

jmuheim commented May 16, 2018

Instead of announcing the currently selected value, it always announces the first radio button:

image

Here the comparison to the example:

image

If you want to play around with the example: https://deploy-preview-37--festive-mcnulty-536163.netlify.com/examples/widgets/autocomplete/_examples/autocomplete-with-radio-buttons/

I think somehow the focus does not stay in the filter input as expected, but is taken to the radio button buttons.

@ralf57
Copy link
Collaborator Author

ralf57 commented May 25, 2018

@jmuheim thanks for reporting.
Apparently there are still some interferences between the 2 libraries.
I will check them out ASAP

@ralf57
Copy link
Collaborator Author

ralf57 commented May 28, 2018

@jmuheim would you have some time to test these Site Search 360 examples (mainly the "Embedding" one) https://sitesearch360.com/docs/example-simple.html
in NVDA/Jaws and let me know if they are ok or not?

@ralf57
Copy link
Collaborator Author

ralf57 commented Jun 6, 2018

@jmuheim As discussed, I've reverted back to using the Site Search 360 provided widget.
Furthermore, I've enabled search results as suggestions (to give more context) and fine-tuned layout and styling. Could you please review it again?

Copy link
Collaborator

@jmuheim jmuheim left a comment

Choose a reason for hiding this comment

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

See my comments. Otherwise: nice! 👍 Thank you.

<button class="search-btn">Suche</button>
<div class="search js-search" role="search">
<h2 class="visuallyhidden">Search</h2>
<h3 class="visuallyhidden">Search</h3>
Copy link
Collaborator

Choose a reason for hiding this comment

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

One too much! ;)

@@ -0,0 +1,137 @@
import $ from 'jquery'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove this file

@@ -0,0 +1,295 @@
import $ from 'jquery'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove this file

@jmuheim jmuheim merged commit 33e922f into master Jun 6, 2018
@jmuheim jmuheim deleted the feature/search branch June 6, 2018 13:07
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.

2 participants