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

[core] fix(PanelStack2): animation direction in controlled mode #4800

Merged
merged 3 commits into from
Jul 12, 2021

Conversation

invliD
Copy link
Member

@invliD invliD commented Jul 5, 2021

Fixes #4686

I have a cherry-pick onto v4's renamed PanelStack ready locally (based on #4799, so not pushing just yet).

@blueprint-bot
Copy link

Fix PanelStack2 animation direction in controlled mode

Previews: documentation | landing | table

@blueprint-bot
Copy link

Fix lint

Previews: documentation | landing | table

@invliD invliD requested a review from adidahiya July 6, 2021 09:21
Copy link
Contributor

@adidahiya adidahiya left a comment

Choose a reason for hiding this comment

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

the gist of the change seems reasonable, but it causes issues when renderActivePanelOnly={false}, the page crashes:

2021-07-09 17 04 12

packages/core/src/components/panel-stack2/panelStack2.tsx Outdated Show resolved Hide resolved
@blueprint-bot
Copy link

Fix import

Previews: documentation | landing | table

@invliD
Copy link
Member Author

invliD commented Jul 12, 2021

@adidahiya That crash is actually not caused by this PR. I've found two issues that would cause a crash due to inconsistent number of hooks called:

  1. Hooks are called after this early return:

    if (stack.length === 0) {
    return null;
    }
    const handlePanelOpen = React.useCallback(

  2. The renderPanel function is called, not added as a JSX.Element. This is the issue the example is running into when using controlled mode (in fact just the 2-line change switching the example to controlled mode reproduces the issue). If a component uses hooks, it must be created as a JSX.Element, otherwise the hook calls are attributed to the parent component. In that case, unless the current panel or all panels together (depending on renderActivePanelOnly) call the same number of hooks, we'll run into that crash.

    {props.panel.renderPanel({
    closePanel: handleClose,
    openPanel: props.onOpen,
    ...props.panel.props,
    } as PanelProps<T>)}

    I believe the three options we have are either (1) pretend renderPanel is a component and call createElement for it, (2) wrap each renderPanel call in a simple FC that essentially takes ownership of potential hook calls, or (3) break typings of renderPanel to disallow passing of an FC itself, always requiring a function. Not sure that one's possible.

Either way, given that the issue already exists, let's fix that in a follow-up.

@adidahiya
Copy link
Contributor

Filed #4807 for follow-up bug

@adidahiya adidahiya changed the title Fix PanelStack2 animation direction in controlled mode [core] fix(PanelStack2): animation direction in controlled mode Jul 12, 2021
@adidahiya adidahiya merged commit 713d524 into develop Jul 12, 2021
@adidahiya adidahiya deleted the sb/panel-stack2-direction branch July 12, 2021 20:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PanelStack controlled mode transitions
3 participants