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

refactor(v2): apply common behavior to dropdowns #3578

Merged
merged 5 commits into from
Oct 14, 2020

Conversation

lex111
Copy link
Contributor

@lex111 lex111 commented Oct 12, 2020

Motivation

I suggest changing the behavior of navbar dropdowns to make it more predictable and standardized.

  • First, we always make a non-clickable link that opens the dropdown itself.
  • Secondly, when accessing from the keyboard, dropdown is opened only when we press the Enter button. This is consistent with the recommendations of a11y, and it's just convenient because we don't have to go through all the child items in dropdown to get to the next navbar item.

FYI, Bootstrap dropdowns works in a similar way, you can look at the Doctrine website (for example) - https://www.doctrine-project.org/

BTW, this PR fixes the annoying bug where we cannot navigate back to the previous navbar items using the keyboard.

ezgif com-gif-maker (2)

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

Current behavior (dropdown opens by pressing Tab, which is not very flexible and hardly we always need it):

ezgif com-gif-maker (1)

New behavior (dropdown opens by pressing Enter):

ezgif com-gif-maker

Related PRs

(If this PR adds or changes functionality, please take some time to update the docs at https://github.com/facebook/docusaurus, and link to your PR here.)

@lex111 lex111 added the pr: breaking change Existing sites may not build successfully in the new version. Description contains more details. label Oct 12, 2020
@lex111 lex111 requested a review from slorber as a code owner October 12, 2020 19:36
@docusaurus-bot
Copy link
Contributor

docusaurus-bot commented Oct 12, 2020

Deploy preview for docusaurus-2 ready!

Built with commit 03ebc7c

https://deploy-preview-3578--docusaurus-2.netlify.app

@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Oct 12, 2020
@lex111
Copy link
Contributor Author

lex111 commented Oct 12, 2020

Strange, locally I have no errors 🤷‍♂️

@slorber
Copy link
Collaborator

slorber commented Oct 14, 2020

thanks, looks better :)

@slorber
Copy link
Collaborator

slorber commented Oct 14, 2020

Do you consider it a breaking change?

@slorber slorber merged commit 5be3471 into master Oct 14, 2020
@lex111
Copy link
Contributor Author

lex111 commented Oct 14, 2020

Do you consider it a breaking change?

Yess, because after that the dropdown button will always be non-clickable. I don't think this is a big deal, but it's a breaking change nonetheless.

@lex111 lex111 added the pr: polish This PR adds a very minor behavior improvement that users will enjoy. label Oct 14, 2020
@slorber
Copy link
Collaborator

slorber commented Oct 14, 2020

oh, didn't see that @lex111

why do you prefer to make it non-clickable?
I see it as a shortcut to the version entry point being displayed.
Also the cursor indicates that we can click on it, but now it does not do anything anymore, so this is a bit confusing.

I also find the behavior of the doctrine website confusing. Why do we need this link to not be clickable?

The Bootstrap dropdown does not feel confusing, because the dropdown only opens on click, not on hover, so the cursor makes sense here.

@lex111
Copy link
Contributor Author

lex111 commented Oct 14, 2020

The Doctrine site is just an example of how Boostrap Dropdown works. I have looked at other sites with dropdowns by hover, and there is almost always link (dropdown button) that is not clickable. Moreover, clickable link makes it difficult to control from the keyboard. In this case, we need to go through all the dropdown items, although we only want to switch to the next navbar item.

I also find the behavior of the doctrine website confusing. Why do we need this link to not be clickable?

That is why I named this behavior as "common". If we have a dropdown that opens on hover, then we need to ensure that they are well accessible from the keyboard.

I see it as a shortcut to the version entry point being displayed.

It's an indicator of the current version, eg.

Also the cursor indicates that we can click on it, but now it does not do anything anymore, so this is a bit confusing.

This will be fixed in Infima.

@slorber
Copy link
Collaborator

slorber commented Oct 14, 2020

Can't we make it clickable, and yet allow to open the link on enter?
Should be possible, by calling preventDefault() maybe?

Removing the cursor would make it consistent, but still find it confusing that it shows the link on the browser bottom bar and that we can open a new tab with right click.

I found this. shortcut convenient, particularly for those that don't have other navbar items on purpose (ie, no docs/api in our case). If we remove it, I guess we'd as well link to # or not use a link at all?

@lex111
Copy link
Contributor Author

lex111 commented Oct 14, 2020

In other words, if we browsing from the keyboard, we ignore the clickable link by pressing Enter key (since it is used to open/close the dropdown), and ordinarily (navigate via mouse), the clickable link works as expected, right?
I'm not sure if this is really better, I still focused on how dropdown on hover usually works (based from other sites/resources about a11y).

@lex111
Copy link
Contributor Author

lex111 commented Oct 14, 2020

Another example - https://v3.vuejs.org/ /https://get.foundation/sites/docs/accordion.html or Gatsby https://www.gatsbyjs.com/ (click/hover on dropdown switch it.)

@slorber
Copy link
Collaborator

slorber commented Oct 15, 2020

You are right that the dropdown button does not usually link to anywhere.
I found it was a convenient shortcut but agree it's not common.

However, if you look at Vue3 ou Gatsby, the dropdown button is a button, not a link (unlike Foundation).

I find it confusing to have a link that shows a browser status bar URL and that we can't click (ie current and Foundation behavior).

I find it better if it's just a button and does not feel like a link, like Vue/Gatsby

So what about using a button instead? (bonus if clicking it actually does something, like giving focus ring, or toggle open/close state...)

@slorber
Copy link
Collaborator

slorber commented Oct 15, 2020

Also, something quite important: for archiving purpose, I'd like to be able to build the site with a single version (like the oldest one) so that it can be deployed as a standalone deployment (will be needed for RN website).

In such case, the dropdown should become a regular, clickable link (because a dropdown with a single version does not make sense)

I think this behavior is not broken (as there's no items array) but just wanted to let you know about the plan ;)

@lex111
Copy link
Contributor Author

lex111 commented Oct 15, 2020

So what about using a button instead? (bonus if clicking it actually does something, like giving focus ring, or toggle open/close state...)

This is quite tricky as we are using highlighting mechanism (isActive prop) that is applied to links (@docusaurus/Link)...

We can handle the click on the dropdown button via the onClick handler, how do you like this idea?

@lex111
Copy link
Contributor Author

lex111 commented Oct 15, 2020

Although I just thought that manually switching the dropdown visibility (using onMouseOver/onMouseOut, imitating hover CSS interaction) if we want to handle a click would be too expensive and difficult.
I would just remove the cursor (cursor:none on dropdown link) when hovering dropdown pseudo-button, and left it as it was. It's much easier and better. WDYT?

@slorber
Copy link
Collaborator

slorber commented Oct 16, 2020

It's hard for me to visualize what would be the result of your suggestions without a preview.

What I'd like in the end is:

  • if it's a link, it should show URL target in browser on hover, and be clickable to navigate
  • if it's not a link, it should not show URL target in browser on hover, and not navigate on click

@lex111
Copy link
Contributor Author

lex111 commented Oct 16, 2020

if it's a link, it should show URL target in browser on hover, and be clickable to navigate

When navigating using keyboard, the user still won't be able to click the link in the dropdown button (because pressing Enter key will control dropdown visibility). Is that okay for you? If so, then I will implement this option.

@slorber
Copy link
Collaborator

slorber commented Oct 16, 2020

That seems acceptable, as there's the link in the dropdown items on which you can enter anyway.

@lex111 lex111 removed the pr: breaking change Existing sites may not build successfully in the new version. Description contains more details. label Oct 16, 2020
@lex111 lex111 deleted the lex111/standard-dropdown-behavior branch October 16, 2020 18:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Signed Facebook CLA pr: polish This PR adds a very minor behavior improvement that users will enjoy.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants