-
Notifications
You must be signed in to change notification settings - Fork 12
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
[Story]: Add checkboxes to the items in the search filters #598
base: develop
Are you sure you want to change the base?
Conversation
…rms, reorganized the checkboxes snippet for multipurpose use
Can you djlint the html files? |
ckanext/ontario_theme/templates/internal/snippets/access_level_checkboxes.html
Show resolved
Hide resolved
ckanext/ontario_theme/templates/internal/snippets/facet_arrangement.html
Outdated
Show resolved
Hide resolved
ckanext/ontario_theme/templates/internal/snippets/facet_arrangement.html
Outdated
Show resolved
Hide resolved
@aleeexgreeen I found the following case where 1 dataset exists at level Restricted but when the Restricted access level is selected with the Open checkbox selected as well (also when all checkboxes are selected), that dataset is not shown and the callout says "No datasets": Steps to reproduce:
|
ckanext/ontario_theme/templates/internal/snippets/facet_list.html
Outdated
Show resolved
Hide resolved
Hi @KatiRG, I don't have the environment or access set up at the moment, but can you revert the change proposed in #598 (comment) and let me know if this is still an issue? |
Thanks @aleeexgreeen ! This solves the problem. I didn't notice the brackets around the second or grouping! I'll revert |
# To show all access levels on dataset counts on | ||
# the homepage and org/group pages | ||
if ('fq' not in search_params) or (fq and not fl): | ||
fq_list = search_params.setdefault('fq_list', []) |
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.
fq_list
is set here to []
but is never used again outside this if
statement
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 because the fq_list
is used with setdefault()
. The default value for fq_list is being set and modified within the if statement, so there is no need to use it outside the if statement, because it is being updated directly into search_params
ckanext/ontario_theme/templates/internal/snippets/search_form.html
Outdated
Show resolved
Hide resolved
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 indentation irregularities in this file that are not caught by djlint for some reason. e.g. in block {% block search_facets %}
and onwards.
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.
You may have a different version of djlint or what you are using is not specifically tailored for this repo. There are files which the command pre-commit run --hook-stage manual format
works from (see .djlintrc
and djlint_rules.yaml
). I say this because when running pre-commit run --hook-stage manual format
there are no formatting errors in the search_form.html
ckanext/ontario_theme/templates/internal/snippets/search_form.html
Outdated
Show resolved
Hide resolved
<div> | ||
{{ h.snippet('snippets/facet_arrangement.html', single_option_facets=single_option_facets, extras={'id':c.group_dict.id}) }} | ||
</div> | ||
<div>{{ h.snippet('snippets/facet_arrangement.html', extras={'id':c.group_dict.id}) }}</div> |
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.
In organization/read.html
, you pass single_option_facet="organization"
to facet_arrangement.html
:
{{ h.snippet('snippets/facet_arrangement.html', extras={'id':c.group_dict.id},
single_option_facet="organization") }}
I'm wondering if you need to do the same for groups. Otherwise, all the groups will be listed in the facet column of an individual group page.
(screenshot from local)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.
A dataset can only have one organization but a dataset can have multiple groups. On the groups page, does it make sense to show what other groups the datasets may be connected to?
<use xlink:href="#ontario-icon-filter"></use> | ||
</svg> | ||
{{ title }} | ||
{% set screen_reader_text = "No search filters applied for "+title+". Options are " %} |
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.
Chrome screen reader is not reading this text. Can you confirm? And what are the options that are supposed to be read out?
{% set screen_reader_text = "No search filters applied for "+title+". Options are " %} | ||
{% if items %} | ||
{% if (items|selectattr("active")|list|length) > 0 %} | ||
{% set screen_reader_text = "Search filters are applied for "+title+"." %} |
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.
Chrome screen reader is not reading this text after a topic is selected. Can you confirm?
</span> | ||
</span> | ||
{% endif %} | ||
{% endfor %} | ||
{% if c.fields|length >= 2 %} |
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 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 doesn't solve the problem, as you can have multiple access levels checked and c.fields|length
will increase. All 3 access levels checked will have the same effect (The displaying of the button is all done in the javascript). I've opted to removing the if statement and leaving the javascript as if.
…r filters button and changed var to const
What this PR accomplishes
search_form.html
Issue(s) addressed
What needs review
Filters sidebar:
Access level Zero results:
Other things needing review:
Tests should be done on individual ministry and group pages and the dataset search page