[docs] Add a shortcut to access the search bar #23959

hmaddisb · 2020-12-11T11:19:17Z

Shortcut CTRL + K for accessing search bar was added and issue with active element gets fixed.

https://deploy-preview-23959--material-ui.netlify.app/getting-started/installation/

mui-pr-bot · 2020-12-11T11:22:22Z

Generated by 🚫 dangerJS against 5de2cd3

oliviertassinari · 2020-12-12T14:07:56Z

@mnajdova I have experimented a bit with using the system over makeStyles in 0e09c6e. I have faced an issue with TypeScript, that I have tried to solve with the diff you can see. However, I gave up on the idea of pushing it further. The results of https://github.com/mui-org/material-ui/blob/next/benchmark/browser/README.md#output made me too worried about the performance. I think that for the migration, better to use the styled() API here. I wanted to name the component anyway.

eps1lon · 2020-12-13T12:13:18Z

docs/src/modules/components/AppSearch.js

        classes={{
          root: classes.inputRoot,
          input: classes.inputInput,
        }}
      />
+      {/* eslint-disable-next-line material-ui/no-hardcoded-labels */}
+      <div className={clsx(classes.shortcut, { 'Mui-focused': focused })}>/</div>


We should display the dual-key keyboard shortcut here instead so that it's more apparent what this is supposed to do. We're currently using the most exotic shortcut.

We are leveraging what GitHub is teaching developers. I think that we better display it this way. Using / is a common convention.

Using / is a common convention.

Please explain how you define common.

"common" as a shortcut our audience has already learned, so they can keep previous habits. e.g. GitHub, Gmail, DocSearch, Twitter, Apollo docs.

How do you know that the audience has learned it?

e.g. GitHub, Gmail, DocSearch, Twitter, Apollo docs.

Please make this exhaustive. I just want to be able to work with you in feature PRs. So far you change you references every time and it would be nice to know if these are actual principles or just smoke screens for "I just prefer it that way".

PS:
Why do you exclude e.g. testing-library.com (or any docosaurus v2? for that matter).

PPS: Could you clarify why we want all 3 shortcuts? On one side you say "use these reference". But some of those I'm not sure where they implement / (gmail) or they only implement / not the other ones.

PPPS:

github: displays and implement /, no CTRL+K

gmail: unclear where they implement it

docsearch: displays CTRL+K, implements / (looks like the same template as testing-library.com)

twitter: does not display / but implements it, no CTRL+K

apollo docs: display and implement /

What standard was applied here that you all consider these as arguments for "display /, implement s, / and CTRL+K"

@eps1lon Considering it's a detail and that we can probably solve my previous concern of visual distraction by reducing the contrast of the displayed shortcut, I think that if you think strongly about it, we can move forward with displaying Ctrl + K by default.

I also think that supporting / and s is important. Maybe we could solve #23959 (comment) with 2. for these two shortcuts and support Ctrl + K with the current detection logic?

I think that we have spent enough time on the topic, relative to its importance.

You have no arguments anymore.

I also think that supporting / and s is important.

Why?

Why?

Developers have learned this shortcut, it's a habit for some. I think that we can improve the UX for them by supporting these shortcuts.

Developers have learned this shortcut

What's your evidence for that? Why does this not apply to ctrl+k?

Ok, let's drop them and wait for developers to complain (maybe they will never do).

eps1lon · 2020-12-13T12:13:34Z

docs/src/modules/components/AppSearch.js

+      const matchKey =
+        ((nativeEvent.ctrlKey || nativeEvent.metaKey) && nativeEvent.key === 'k') ||
+        nativeEvent.key === '/' ||
+        nativeEvent.key === 's';


Why do we need 3 keyboard shortcuts?

Especially s is problematic. The WCAG guideline to avoid single keys exists for a reason. For example, try our Select components type-ahead feature. On https://deploy-preview-23959--material-ui.netlify.app/components/selects/#basic-select focus the select, open it, and press s. The searchbox will now take focus and our trapfocus will return it to the container i.e. it will focus an element that is not matching what the user typed. On next nothing happens as expected (https://next--material-ui.netlify.app/components/selects/#basic-select).

Oh nice, we need to improve the logic that filters active elements.

WCAG guideline to avoid single keys exists for a reason.

The reason seems to be about speech users, by a large margin. They dedicate a lot of the content to it. I couldn't find our conflict problem covered.

Regarding the solution, I could find 3 options:

We ignore it.

We only handle the keyboard shortcut when body and main are focused.

We add .defaultMuiPrevented to signal that the event was already handled.

Else?

I'm happy with any of these, maybe a slight preference for 2 as a quick win. Thoughts?

We ignore it.

So we're re-opening the discussion about WCAG again. We settled on AA by default but it seems like you're going to reject every single rule as I'm opening them. What is your principled approach to WCAG to which I can hold you accountable? You're always very concerned with ROI but if we don't make decision we're wasting a lot of time in discussions.

We settled on AA by default

@eps1lon I would add a distinction: we settled on it for the components. Did we discuss the documentation yet? I can't remember it. I think that, so far, we fixed issues on the documentation that were preventing correctly audit the accessibility of the components, but without having a strong focus on the documentation itself. I think that it deserves a different discussion because the incentives are not obviously identical. For the component, the motivation is that developers need it for the end-users and the organizations from legal perspectives.

I think that it's similar to the discussion about screenreaders support. We didn't talk about it initially, but later on, we said that we would support the most recent version of the most common screen readers, as well as document it in https://material-ui.com/getting-started/supported-platforms/. At least, from what I recall.

You're always very concerned with ROI but if we don't make a decision we're wasting a lot of time in discussions.

You are right, one of my concerns with WCAG is the return on investment. I think that there is a tradeoff to find between doing enough to be leading the field and not doing too much, sinking resources. My second concern is when it requires harming the UX of the majority of the people. So far, it felt that working on WCAG improved the UX most of the time. The cases where it goes against the UX of unimpaired users are not frequent. Sometimes it's by a small margin, like here. I think that we should be cautious when it's significant.

updated

eps1lon

Accidentally dismissed my review. The unresolved conversation is not stale.

feedback taken into account

[docs] Add a shortcut to access the search bar

f4eeb54

github-actions bot mentioned this pull request Dec 11, 2020

Update snapshots for pr/23959 eps1lon/mui-scripts-incubator#1485

Closed

oliviertassinari self-assigned this Dec 12, 2020

polish shortcut

2a6e86d

oliviertassinari added the docs Improvements or additions to the documentation label Dec 12, 2020

oliviertassinari removed their assignment Dec 12, 2020

vercel bot deployed to Preview December 12, 2020 13:52 View deployment

oliviertassinari approved these changes Dec 12, 2020

View reviewed changes

oliviertassinari requested a review from mbrookes December 12, 2020 22:24

mbrookes approved these changes Dec 12, 2020

View reviewed changes

oliviertassinari requested a review from eps1lon December 13, 2020 00:04

eps1lon previously requested changes Dec 13, 2020

View reviewed changes

This comment has been minimized.

Sign in to view

review

ca1240c

vercel bot deployed to Preview December 14, 2020 22:28 View deployment

oliviertassinari requested a review from eps1lon December 14, 2020 22:28

eps1lon previously requested changes Dec 15, 2020

View reviewed changes

Remove s and / shortcuts

5de2cd3

vercel bot deployed to Preview December 15, 2020 12:40 View deployment

oliviertassinari requested a review from eps1lon December 15, 2020 12:53

eps1lon approved these changes Dec 15, 2020

View reviewed changes

oliviertassinari merged commit de20bfc into mui:next Dec 15, 2020

eps1lon mentioned this pull request Dec 27, 2020

Update keyboard shortcut for search eps1lon/mui-scripts-incubator#1609

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[docs] Add a shortcut to access the search bar #23959

[docs] Add a shortcut to access the search bar #23959

hmaddisb commented Dec 11, 2020 •

edited by oliviertassinari

mui-pr-bot commented Dec 11, 2020 •

edited

oliviertassinari commented Dec 12, 2020 •

edited

eps1lon Dec 13, 2020

oliviertassinari Dec 13, 2020

eps1lon Dec 13, 2020

oliviertassinari Dec 13, 2020 •

edited

eps1lon Dec 13, 2020 •

edited

oliviertassinari Dec 14, 2020 •

edited

eps1lon Dec 15, 2020

oliviertassinari Dec 15, 2020 •

edited

eps1lon Dec 15, 2020

oliviertassinari Dec 15, 2020 •

edited

eps1lon Dec 13, 2020

oliviertassinari Dec 13, 2020

oliviertassinari Dec 13, 2020 •

edited

eps1lon Dec 15, 2020 •

edited

oliviertassinari Dec 15, 2020 •

edited

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

eps1lon left a comment

[docs] Add a shortcut to access the search bar #23959

[docs] Add a shortcut to access the search bar #23959

Conversation

hmaddisb commented Dec 11, 2020 • edited by oliviertassinari

mui-pr-bot commented Dec 11, 2020 • edited

oliviertassinari commented Dec 12, 2020 • edited

eps1lon Dec 13, 2020

Choose a reason for hiding this comment

oliviertassinari Dec 13, 2020

Choose a reason for hiding this comment

eps1lon Dec 13, 2020

Choose a reason for hiding this comment

oliviertassinari Dec 13, 2020 • edited

Choose a reason for hiding this comment

eps1lon Dec 13, 2020 • edited

Choose a reason for hiding this comment

oliviertassinari Dec 14, 2020 • edited

Choose a reason for hiding this comment

eps1lon Dec 15, 2020

Choose a reason for hiding this comment

oliviertassinari Dec 15, 2020 • edited

Choose a reason for hiding this comment

eps1lon Dec 15, 2020

Choose a reason for hiding this comment

oliviertassinari Dec 15, 2020 • edited

Choose a reason for hiding this comment

eps1lon Dec 13, 2020

Choose a reason for hiding this comment

oliviertassinari Dec 13, 2020

Choose a reason for hiding this comment

oliviertassinari Dec 13, 2020 • edited

Choose a reason for hiding this comment

eps1lon Dec 15, 2020 • edited

Choose a reason for hiding this comment

oliviertassinari Dec 15, 2020 • edited

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

eps1lon left a comment

Choose a reason for hiding this comment

hmaddisb commented Dec 11, 2020 •

edited by oliviertassinari

mui-pr-bot commented Dec 11, 2020 •

edited

oliviertassinari commented Dec 12, 2020 •

edited

oliviertassinari Dec 13, 2020 •

edited

eps1lon Dec 13, 2020 •

edited

oliviertassinari Dec 14, 2020 •

edited

oliviertassinari Dec 15, 2020 •

edited

oliviertassinari Dec 15, 2020 •

edited

oliviertassinari Dec 13, 2020 •

edited

eps1lon Dec 15, 2020 •

edited

oliviertassinari Dec 15, 2020 •

edited