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

[VUFIND-1731] Aria hide sliders and replace slider library #4238

Open
wants to merge 11 commits into
base: dev
Choose a base branch
from

Conversation

ThoWagen
Copy link
Contributor

@ThoWagen ThoWagen commented Feb 11, 2025

There is not straightforward way to aria hide the slider using the bootstrap-slider library and although the library currently works with bootstrap 5 it officially only supports bootstrap 3 and 4. The developers have stated that they do not plan to support bootstrap 5 seiyria/bootstrap-slider#961 (comment).

For that reason I replaced the library with nouislider https://github.com/leongersen/noUiSlider.

With that library it is quite easy to aria hide the slider.

TODO

  • Update changelog when merging
  • Resolve VUFIND-1731 when merging

@demiankatz demiankatz added this to the 11.0 milestone Feb 11, 2025
@demiankatz demiankatz added accessibility architecture pull requests that involve significant refactoring / architectural changes labels Feb 11, 2025
Copy link
Member

@demiankatz demiankatz left a comment

Choose a reason for hiding this comment

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

Thanks, @ThoWagen -- a few suggestions and comments based on an initial read-through; I haven't done hands-on testing yet. I'll see if @sturkel89 has time to try this out for UI-oriented testing and feedback.

@demiankatz demiankatz requested a review from sturkel89 February 11, 2025 16:17
Copy link
Contributor

@sturkel89 sturkel89 left a comment

Choose a reason for hiding this comment

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

In my initial testing, this feature seems to work well.

The new control adds up and down arrow functionality within each date entry box, as an additional way to tick the years up and down without having to use the keyboard or number pad:

image

I wasn't sure what to test in terms of settings in facets.ini, and I wasn't sure whether it was important to look at this in both themes. Please let me know.

@demiankatz
Copy link
Member

demiankatz commented Feb 12, 2025

Thanks, @sturkel89!

I don't think you need to do anything more to test this PR for now. I've suggested that @ThoWagen make a couple of minor technical adjustments. Maybe after he has completed that work, you can test again using the theme you did not test with before, just for good measure (though I don't expect there to be major differences between themes).

It also occurred to me that it would be helpful to have testing notes on how to exercise all of the other types of special range facets, so I added them to the wiki... but none of the ranges other than dates use sliders, so testing those is not necessary for this PR. It's just something that might be useful to have in the reportoire for future testing.

@ThoWagen
Copy link
Contributor Author

ThoWagen commented Feb 12, 2025

The only setting coming to my mind that could be worth testing is switching from default_side_recommend[] = SideFacets:Results:CheckboxFacets to default_side_recommend[] = SideFacetsDeferred:Results:CheckboxFacets.

It works for me but I'm not sure if it is worth adding a note for it to the wiki.

@demiankatz
Copy link
Member

The only setting comes to my mind that could be worth testing is switching from default_side_recommend[] = SideFacets:Results:CheckboxFacets to default_side_recommend[] = SideFacetsDeferred:Results:CheckboxFacets.

I works for me but I'm not sure if it is worth adding a note for it to the wiki.

Very good point, it's always worth testing the deferred side facets in parallel with the "regular" ones!

Copy link
Member

@demiankatz demiankatz left a comment

Choose a reason for hiding this comment

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

Thanks for the improvements, @ThoWagen, this continues to look great. See below for a few new comments on the latest round of code.

Copy link
Member

@demiankatz demiankatz left a comment

Choose a reason for hiding this comment

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

Thanks, @ThoWagen! I think we're nearly done, but I had one last thought (see below) which might or might not be worth following up on. No pressure to change this if you disagree -- I'd just like to get your opinion (and perhaps those of others as well).

@demiankatz demiankatz requested a review from sturkel89 February 13, 2025 16:29
@demiankatz
Copy link
Member

Thanks, @ThoWagen, this all looks good to me now, so it's ready for @sturkel89's next round of testing (covering both themes, and checking the slider in EDS, Solr, and on the advanced search page). Once she gives the thumbs-up, I'll approve and merge!

Copy link
Contributor

@sturkel89 sturkel89 left a comment

Choose a reason for hiding this comment

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

I've checked this PR in both themes and against test and dev. I found some issues, including an apparently pre-existing issue.

Aside from the issues listed below, the date slider works fine as a search results filter in both Solr and EDS in both themes. You can type years into both slots and click Set, and the filter is applied. You can also use the sliders to select the years, and/or you can use the arrows to adjust the years. When you click Set, the filter is applied.


Arrows in To: box reset value in From: box

I've identified a weird behavior relating to the arrows inside the new date slider. This issue is present in every situation: Solr, EDS, and Advanced Search.

When I set a value for the From: year, and then click the up arrow in the To: box, the year in the From: box to reset to 1, and the value in the To: box populates with the number that I had originally entered in the From: box. See illustration.

image


Advanced Search: Clear button doesn't work for date slider

UPDATE: this is now fixed.

In the test branch, the Clear button does not clear any aspect of the selected date (the text entered or the sliders). Either Clear button correctly empties the values the user sets for every other field in Advanced Search, but the Clear buttons have no effect on the year slider.

image

On dev, the Clear button clears the values from the boxes. However, the sliders stay slid! But when you run the search, date limits are not set.

image


Advanced Search: both values must be entered before sliders slide

UPDATE: this is now fixed.

On test, when you set a From: year, the slider does not move. You need to set a To: year in order for either slider to move.

image

On dev, the slider moves immediately when you enter a From: year.

image


Pre-existing problem: on narrow screens, date filter does not return you to top of results page.

UPDATE: this is now fixed.

In mobile or narrow screens, when you click Refine Results, you are brought to the list of filters. When you set almost any filter, you are popped back up to the top of the screen to view the results.

With the date slider, when you set the date filter, you are left in facet view (with the Refine Results heading at the top of the screen) and have to manually scroll up to see the search results part of the page.

This seems to be an existing bug, as I see it in test and dev, and with either side facets deferred or not.

Copy link
Member

@demiankatz demiankatz left a comment

Choose a reason for hiding this comment

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

#4261 has fixed the problem that @sturkel89 reported with mobile range facets. However, it seems to have introduced a conflict here. @ThoWagen, do you mind resolving? Please let me know if I can help!

@ThoWagen
Copy link
Contributor Author

Thanks @sturkel89!

Regarding "Arrows in To: box reset value in From: box":

This is intentional. The values are swapped after they have been changed when the "From" is larger that "To". This also happens if you type the values with the keyboard. Note that they only swap after the input is done. I.e. if you unfocus the input after typing or after you release the mouse button when clicking the arrows. If you believe that this behavior is more confusing that helpful I can remove it again.

The rest should be fixed now.

@demiankatz demiankatz requested a review from sturkel89 February 24, 2025 20:15
Copy link
Contributor

@sturkel89 sturkel89 left a comment

Choose a reason for hiding this comment

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

@ThoWagen - I see that the three other items have been fixed - thanks! I noted the resolution of those issues in my review above.

As for the "Arrows in To: box reset value in From: box" issue, I see what you mean. If I type 1940 into the first box, then 1930 in the second box, when I click Set the two values reverse to the logical "from" the earlier date "to" the later date. That is good.

However, if I type 1940 into the From: box, then click the up arrow in the To: box, I expect the To: box to immediately populate with 1941! The current behavior is surprising, in that I specified I want 1940 and later dates, and what happens is that the time frame is immediately switched from the earliest possible date up until 1940 -- mutually exclusive with what I had desired.

I don't know if you can do something to change that behavior while also having the system reverse dates that are entered into the opposite boxes (which is desired behavior).

Thanks for helping to think this through!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accessibility architecture pull requests that involve significant refactoring / architectural changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants