-
-
Notifications
You must be signed in to change notification settings - Fork 995
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
Added documentation type filtering to search. #1935
Conversation
17aba69
to
8febaf6
Compare
567f65a
to
581467e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the idea, but I would try to centralize the list of type in a single dictionary (in search.py
) to pass to the template to dynamically build the list of tabs. I would also move the search logic into the manager method by introducing a new search attribute so as to keep all the logic in one place.
@@ -32,6 +32,13 @@ <h2>{% trans "No search query given" %}</h2> | |||
|
|||
{% if query %} | |||
<div id="docs-content"> | |||
<div role="list" class="tabs"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would use the dict of types to dynamically build the list of tabs.
From accessibility point of view, do you think can we use a better semantic HTML tags? I don't know if <nav>
can be a better choice, I'm not an a11y expert, but maybe we can ask for help to our @django/accessibility team. It would be great to start moving the website from an old list of <div>
or <span>
to a more modern semantic website.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have updated to use <nav>
and removed the naming of "tabs" (these aren't real tabs just look a little bit like them). Would love the accessibility team to check this 💯
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, since we're changing the UX a review from a11y point-of-view would be great.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are a couple of possibilities and I think either are fine.
nav
is great because we're... navigating. However...
Because the text talks about filtering the results, a search
landmark could be another option. But in this case I think it should encompass both the search bar and the tabs. I suspect nav
could also be used in this case. I would consider this option more of a nice to have if the implementation would be annoying.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Due to the way the page is structured, I can't have the tabs and the input in the same <search>
landmark.
So I have done the following:
- updated the original search form to be within a
<search>
and added anaria-label
of "Search the documentation" - updated the filters to be in a
<search>
HTML tag witharia-label
of "Filter the current search results by documentation type". This is only exists on a search result page when something was searched
I checked and from what I have read, having two <search>
html tags on a page is fine as long as it is clear what does what. So I think this is a reasonable approach, we can always change it later
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's probably good enough but it does feel a bit weird I think if they are both contributing to the same search results.
33f19bc
to
267865d
Compare
8186495
to
abffd90
Compare
1c6f8df
to
d4597a7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did a partial review, focussing on the HTML specifically.
Overall, I am happy we are getting improved search results!
docs/templates/docs/search_form.html
Outdated
@@ -1,11 +1,12 @@ | |||
{% load i18n %} | |||
<search class="search form-input" aria-label="{% trans 'Search the documentation' %}"> | |||
<form action="{% url 'document-search' version=version lang=lang host 'docs' %}"> | |||
<label class="visuallyhidden" for="id_q">Search:</label> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would argue that this label is useful for sighted users as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have put all of the suggestions for improvement of the search form in afff9ed
This now renders as:
<search class="search form-input" aria-labelledby="docs-search-label">
<form action="http://docs.localhost:8000/en/5.1/search/">
<label id="docs-search-label" class="visuallyhidden" for="id_q">Search 5.1 documentation</label>
<input type="search" name="q" placeholder="Search 5.1 documentation (Ctrl + K)" id="id_q">
<button type="submit">
<i class="icon icon-search" aria-hidden="true"></i>
<span class="visuallyhidden">Submit</span>
</button>
</form>
</search>
The placeholder was acting as a label for sighted users, I have now put that in the "hidden" label for screen reader users. This is also translated.
The "Ctrl + K" addition appears to be added via JavaScript... let's pretend we didn't see that for now 😅
I don't want to change this existing search form visually in this PR, but happy to do quick accessibility wins as I'm "in the area"
docs/templates/docs/search_form.html
Outdated
@@ -1,11 +1,12 @@ | |||
{% load i18n %} | |||
<search class="search form-input" aria-label="{% trans 'Search the documentation' %}"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we use {% translate %}
instead of {% trans %}
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, updated
Made a new commit for a small clean-up: 0ebb6cc
bca730f
to
5e71f43
Compare
a70becb
to
4f5bcd4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking amazing! I know I'll be using that feature a ton and I can't wait for it to be live.
I've left some non-essential suggestions, mostly around using an actual enum/Textchoices instead of a dict. It doesn't change much fundamentally but I think it makes sense here (I went for a FULL_CAPS
name in singular, but I'm not sure if that's the best practice, maybe an AlternateCase
name would be better? 🤔 ).
Also, I realize this is an annoying change to make and it brings no value code-wise, but I feel that "type" is a bit of a meaningless word (and it being a builtin function in python makes it annoying to use as a variable name). What do you think of "category", "topic" or maybe even "flavor" (I'll even look the other way if you spell it "flavour" 👀 )
3a2a875
to
8380c9d
Compare
8380c9d
to
d7a10f1
Compare
Thank you Marijke Luttekes for the review.
Thank you to Paulo Melchiorre, Tom Carrick, Marijke Luttekes, and Baptiste Mispelon for the reviews.
d7a10f1
to
e4d59ba
Compare
In order to make it easier to refine the search results for the type of information you want, added a filter by docs categories. There are more categories (e.g. "Internals") but I think it is better to only include the most likely categories
This slightly relates to #1628
Mobile view
Desktop view