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): animate dropdown properly #3603

Merged
merged 2 commits into from
Oct 21, 2020
Merged

fix(v2): animate dropdown properly #3603

merged 2 commits into from
Oct 21, 2020

Conversation

lex111
Copy link
Contributor

@lex111 lex111 commented Oct 16, 2020

Motivation

Currently, in Infima we have height transition CSS property, but it won't work unless the element's height is explicitly specified.
This PR sets the dropdown height at click time so that the CSS transition can work as expected.

The desktop implementation looks trickier because we can have nested dropdowns, so the final height can be dynamic (depending on whether the nested dropdown is expanded or collapsed).

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

Preview.

Desktop Mobile
ezgif com-gif-maker (5) ezgif com-gif-maker (6)

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: bug fix This PR fixes a bug in a past release. label Oct 16, 2020
@lex111 lex111 requested a review from slorber as a code owner October 16, 2020 23:41
@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Oct 16, 2020
@docusaurus-bot
Copy link
Contributor

docusaurus-bot commented Oct 16, 2020

Deploy preview for docusaurus-2 ready!

Built with commit 93ebafb

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

@@ -64,6 +64,23 @@ function DocSidebarItemCategory({
return isActive ? false : item.collapsed;
});

const menuListRef = useRef<HTMLUListElement>(null);
const [menuListHeight, setMenuListHeight] = useState<string | undefined>(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Of course, we can eliminate to create a new state for storing the height and set it via menuListRef.current.style.height, but it seems to me better to do this using React.

@slorber
Copy link
Collaborator

slorber commented Oct 19, 2020

Not sure exactly why but the DocsSidebar transition does not always work reliably. Sometimes there's some jank. Also it seems to not work great for multiple nesting levels (see Guides collapsing transition)

@lex111
Copy link
Contributor Author

lex111 commented Oct 19, 2020

What exactly is wrong? I need screenshots or videos because I don't see any issues with transition.

@slorber
Copy link
Collaborator

slorber commented Oct 19, 2020

The multi-level collapsing of Guides seem to collapse subcategories first:

guides

Here sometimes the category collapses without anim. Not sure how to reproduce this consistently but it happens maybe 5% or 10% of the time when I click randomly on a category:

no-anim

@lex111
Copy link
Contributor Author

lex111 commented Oct 19, 2020

The multi-level collapsing of Guides seem to collapse subcategories first:

I don't understand anyway, what is the problem?

Here sometimes the category collapses without anim. Not sure how to reproduce this consistently but it happens maybe 5% or 10% of the time when I click randomly on a category:

Can't reproduce this.

Anyway, I do the animation (transition) with CSS, I only set the height so that the CSS transition is possible 🤷‍♂️

@slorber
Copy link
Collaborator

slorber commented Oct 19, 2020

The 2nd is harder to reproduce. Sometimes I can't even make it happen 🤪

But the first is easy: when you click on "Guides" while it's expanded, the "Docs" subcategory also collapses, but its collapsing is not animated (Introduction / MD Features / Versioning disappear instantly). Maybe slowing down the animation would make it easier to see.

Is it expected that the subcategory also collapse when a parent collapse? That seems weird to me.

@lex111
Copy link
Contributor Author

lex111 commented Oct 20, 2020

Yes, this is how CSS transition works, I don't see a big issue with that.

However, I did find bug when collapsed back nested dropdowns. I ended up simplifying the code, so the transition should work better now.

}

:global(.menu__list-item--collapsed) :global(.menu__list) {
height: 0px !important;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since we set the height dynamically via style attr, it will be prioritized over the class name, so to avoid this we had to use !important rule. This won't be moved to Infima, only for Docusaurus code base.

@slorber
Copy link
Collaborator

slorber commented Oct 21, 2020

let's merge this for now and see if users give us feedbacks

@slorber slorber merged commit ce06e10 into master Oct 21, 2020
@lex111 lex111 deleted the lex111/animate-dropdown branch October 21, 2020 16:49
@lex111 lex111 added this to the v2.0.0-alpha.67 milestone Nov 17, 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