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

Filter sort #117

Merged
merged 18 commits into from
Jun 21, 2019
Merged

Filter sort #117

merged 18 commits into from
Jun 21, 2019

Conversation

Heydon
Copy link
Contributor

@Heydon Heydon commented Jun 20, 2019

Includes the search component originally in #109 and already reviewed

@Heydon Heydon mentioned this pull request Jun 20, 2019
@micmath
Copy link
Contributor

micmath commented Jun 20, 2019

@Heydon, I feel conflicted about this PR. Can you help resolve that?

@Heydon
Copy link
Contributor Author

Heydon commented Jun 20, 2019

@micmath I'll attempt a rebase. Hold onto your hat!

@Heydon
Copy link
Contributor Author

Heydon commented Jun 20, 2019

@micmath I can easily fix the conflicts, but I noticed that merging would delete the tables component. I'm not sure why this would be (or what other work would be lost) at the moment.

@Heydon
Copy link
Contributor Author

Heydon commented Jun 20, 2019

@micmath Okay, I've fixed this. Sorry it's now such a big PR. I can try squashing if you need. The only relevant files for review are the ones relating to Filter/Sort.

@micmath
Copy link
Contributor

micmath commented Jun 20, 2019

I’ll have a go, but you may need to go back after and fix a few things.

@Heydon
Copy link
Contributor Author

Heydon commented Jun 20, 2019

@micmath Nothing should break, since the search, tables, and infographics stuff are already merged in. But it's a lot in one place, sorry.

@micmath
Copy link
Contributor

micmath commented Jun 20, 2019

You are right there weren’t any merge conflicts. I made some changes (see below) so you will need to git pull those.

Playing around with the filter demo on the actual documentation page, I noticed that pressing a several filter tabs in series was not a great experience: clicking <a href=".?filter=filter+1">Filter 1</a> for example, causes a page reload. So the content in the browser window appears to jump to the top of the page after each filter is changed. I tried to beat that by throwing focus to the “content” in the filter pane, but that invoked the pretty scrolling transitions, making the experience of clicking several filter tabs in sequence feel like a funfair ride.

The actual answer is that we shouldn’t be reloading whenever the filter is changed, in my opinion. Ideally that would involve a spinner and a XMLHttpRequest call to load the filtered content in place. If you agree, do you think you can rework that demo so it just updates the filter-content, SPA-style, when JS is enabled, and falls back to doing a page reload only for the no JS sitch?

@Heydon
Copy link
Contributor Author

Heydon commented Jun 20, 2019

@micmath Ah, yes. Some demos don't really suit the inline/embedded versions.

I did my best to emulate the behaviour of the Academy search, which I liked because it didn't require JavaScript. If JavaScript was involved, we wouldn't want to use links. It would be a form and radio buttons, most likely.

The filter constructor JS only adds the 'More' menu enhancement. The load more functionality would use JavaScript, but only for successive clicks of 'Load more' after the initial filter/sort endpoint is loaded.

That just leaves the bespoke JS to emulate whole page loads in the single page demo.

@micmath
Copy link
Contributor

micmath commented Jun 20, 2019

I think a better example to look at is Food. For example a search for chocolate can be filtered in many ways. Ignoring the oddity of having menus in the filter tabs, they do manage to keep keyboard focus on the filter control element, even after the control is activated. So, for example I can tab over to the “Dairy-free” filter option, activate it (thus triggering the result to be updated in page), and then just keep tabbing along from there.

If JS is available, can the demo just update the content in the area that shows the results, without reloading the whole page please?

@micmath
Copy link
Contributor

micmath commented Jun 20, 2019

Thinks history.pushState() and/or history.replaceState() might be involved…

@micmath
Copy link
Contributor

micmath commented Jun 20, 2019

If that sounds like a donkey’s job in JS, I can break out my code editor and give some help. I expect @simonsinclair could lend a hand too.

@Heydon
Copy link
Contributor Author

Heydon commented Jun 20, 2019

@micmath I was only aware of the Academy pattern, and implemented since I liked it that way (although I fixed the fact that the sorting was not at all keyboard accessible). What you are proposing is a completely different, and more complex, pattern (I think).

The Food implementation is broken without JavaScript (the filters appear but do nothing). It might be possible to progressively enhance the links-based version I have already done to create radio button semantics (ARIA) and behaviors. Thoughts?

@micmath
Copy link
Contributor

micmath commented Jun 21, 2019

I must not have explained what I was asking for properly, because if I had there is no way you would say it was "completely different, and more complex". How did you fix the fact that the sorting was not at all keyboard accessible by the way? I don't see that change in github yet.

My suggestion was/is basically that filters work like an asynchronous version of Tabs. So, just like Tabs, the page shouldn't reload upon a tab (read "filter") being selected. In fact you can ignore the asynchronicity aspect too and leave that as an exercise for the library implementor, I just thought that might be a fun/interesting nut to document.

@Heydon
Copy link
Contributor Author

Heydon commented Jun 21, 2019

How did you fix the fact that the sorting was not at all keyboard accessible by the way? I don't see that change in github yet.

I did it by not trying to emulate the inaccessible dropdown at all. Instead, the two links appear side-by-side, with the 'current' one highlighted via aria-current="page". Regardless of technical accessibility, I believe using a dropdown for two or three sorting options is an anti-pattern.

Looking at the main search page itself (at https://www.bbc.co.uk/search), it appears to only expose filters with JS enabled. However, the pattern is in accordance with my implementation: it uses links, and whole page reloads, to access each filter. I'm concerned that diverging from this pattern too much will mean creating an interface that isn't compatible with the backend/API/architecture, or whatever's there. But maybe I shouldn't worry about that?

Another thing I noticed is that the focus order has been augmented, so that the search field is first to be focused, before the skip links even. This I would recommend against, but was probably instated to improve full-page-load filtering by reducing the number of tabs to the search functionality.

Your proposed JS-based solution would, as you said, address this — because using XHR would allow us to just change the contents, and keep focus on the current button. If we were to build this version on top of a basic link-based version, it would be necessarily more complex. It's possibly doable, but I'd probably have to enhance the links into ARIA radio buttons, which would mean supporting arrow key selection as well. Doing this across the principle links and 'more menu' would probably be a very confusing experience, since a number of the radio controls would be hidden behind the "More" button. There's also the concern that, without looking like radio buttons, sighted keyboard users would not be aware of the expected interaction paradigm.

It would also be tricky to implement: would pressing the right arrow key on the right-most principle link both open the "More" menu and focus the first of its controls? Perhaps enhancing to basic buttons, and using aria-pressed would be relatively better? We couldn't really go full tabs because the "More" menu breaks that (already complex and confusing, as the recent research showed) pattern up.

In any case, all of this would be to save the user a few tab stops for successive filter/sorting selections. Not an inconsiderable improvement in terms of convenience, I agree, but I think the simplicity of the link-based interaction paradigm is both more clear, and more robust. Not forgetting that a page load automatically means the <title> is announced, which would/should name the chosen filter/sort option (I should add this recommendation to the doc' actually). To provide similar context without a page load would mean cludging in a live region: more complexity still, and not as reliable or conventional.

Whew. Sorry for the long comment. Those are all my thoughts so far!

@Heydon
Copy link
Contributor Author

Heydon commented Jun 21, 2019

@micmath All yours!

@micmath micmath merged commit 8f9196e into master Jun 21, 2019
@micmath micmath deleted the filter-sort branch June 21, 2019 14:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
2 participants