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

fix(v2): navbar dropdown opened with tab, not closing on click outside #3240

Merged
merged 8 commits into from
Aug 11, 2020

Conversation

Ako92
Copy link
Contributor

@Ako92 Ako92 commented Aug 7, 2020

Motivation

There is a Problem on documentation of docusaurus with drop down in header.
it fixed in this pull request.

Have you read the Contributing Guidelines on pull requests?

Yes I read it.

Test Plan

Just start the project and check the drop down in navigation. press tab key from first and see that the drop down opens tab navigation navigate through it.

UFTbER0GEG

@docusaurus-bot
Copy link
Contributor

docusaurus-bot commented Aug 7, 2020

Deploy preview for docusaurus-2 ready!

Built with commit b145dde

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

@Ako92 Ako92 changed the title Tab behavior on header dropdown Fix: Tab behavior on header dropdown Aug 7, 2020
Copy link
Collaborator

@slorber slorber left a comment

Choose a reason for hiding this comment

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

thanks

seems to work fine, what about a few code changes ?

@@ -61,6 +63,22 @@ function NavLink({
}

function NavItemDesktop({items, position, className, ...props}) {
const dropDownRef = React.useRef<HTMLDivElement>(null);
const dropDownMenuRef = React.useRef<any>(null); // TODO should find better solution for this. anything else will get a error when retrieve children.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Doesn't something like <UlHtmlElement | null> works?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tested lots of solutions! Anything will get this error on focus method of childNodes!
image

I can add <HtmlUlistElement | any>. is it okey?

@Ako92
Copy link
Contributor Author

Ako92 commented Aug 7, 2020

It's okay. What code changes?

@Ako92
Copy link
Contributor Author

Ako92 commented Aug 8, 2020

optional chaining added and pushed again
type checking improved. but not good enough yet!
But i don't have better solution for it!

@Ako92 Ako92 requested a review from slorber August 8, 2020 10:52
Copy link
Collaborator

@slorber slorber left a comment

Choose a reason for hiding this comment

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

CI fails

@Ako92
Copy link
Contributor Author

Ako92 commented Aug 8, 2020

Problem of type checking solved by add type on focus() call site.
Sorry about missing comma on package.json, it happened because I resolved the package.json conflict with github inline editor. Fixed
CI checks successfully done.

@Ako92 Ako92 requested a review from slorber August 8, 2020 15:49
@slorber slorber added the pr: bug fix This PR fixes a bug in a past release. label Aug 11, 2020
@slorber slorber changed the title Fix: Tab behavior on header dropdown fix(v2): navbar dropdown opened with tab, not closing on click outside Aug 11, 2020
@slorber
Copy link
Collaborator

slorber commented Aug 11, 2020

thankss :)

@slorber slorber merged commit ee6dee7 into facebook:master Aug 11, 2020
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: bug fix This PR fixes a bug in a past release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants