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] feat(PanelStack): add renderCurrentPanelOnly prop #3768

Merged
merged 2 commits into from
Feb 1, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion packages/core/src/components/panel-stack/_panel-stack.scss
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@
display: flex;
flex-shrink: 0;
align-items: center;
z-index: 1;
Copy link
Member

Choose a reason for hiding this comment

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

Why was this removed? This breaks the box-shadow from the next line when the panel view's content has a background.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@invliD It was quite a while ago, but it seems it got moved to .#{$ns}-panel-stack-view below. If it's a problem at the moment, I think you should file a new issue about that.

Copy link
Member

Choose a reason for hiding this comment

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

I did see it got added to the view, but that doesn't explain why it was removed here. I added it here for exactly this reason when I wrote the panel stack; guess I'll just add it back…

box-shadow: 0 1px $pt-divider-black;
height: $pt-grid-size * 3;

Expand Down Expand Up @@ -48,6 +47,7 @@
@include position-all(absolute, 0);
display: flex;
flex-direction: column;
z-index: 1;

// border between panels, visible during transition
margin-right: -1px;
Expand All @@ -59,6 +59,10 @@
.#{$ns}-dark & {
background-color: $dark-gray4;
}

&:nth-last-child(n + 4) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

All but last 3. Why 3? When one removes the top panel, the bottom two should be visible to animate properly. The rest can be hidden as a layering optimization.

Copy link
Contributor

Choose a reason for hiding this comment

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

The rest can be hidden as a layering optimization.

Can you provide any evidence that we actually need this optimization?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, it's not really needed - I wanted to make it in align with the Tabs implementation (JSX, SCSS).

display: none;
}
}

// PUSH transition: enter from right (100%), existing panel moves off left.
Expand Down
32 changes: 24 additions & 8 deletions packages/core/src/components/panel-stack/panelStack.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,12 @@ export interface IPanelStackProps extends IProps {
*/
onOpen?: (addedPanel: IPanel) => void;

/**
* If false, PanelStack will render all panels in the stack to the DOM, allowing their React component trees to maintain state as a user navigates through the stack. Panels other than the currently active one will be invisible.
* @default true
*/
renderCurrentPanelOnly?: boolean;
Copy link
Contributor

Choose a reason for hiding this comment

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

let's change this name to mirror a similar prop for the Tabs component... renderActivePanelOnly


/**
* Whether to show the header with the "back" button in each panel.
* @default true
Expand Down Expand Up @@ -93,7 +99,7 @@ export class PanelStack extends AbstractPureComponent2<IPanelStackProps, IPanelS
);
return (
<TransitionGroup className={classes} component="div">
{this.renderCurrentPanel()}
{this.renderPanels()}
</TransitionGroup>
);
}
Expand All @@ -110,25 +116,35 @@ export class PanelStack extends AbstractPureComponent2<IPanelStackProps, IPanelS
}
}

private renderCurrentPanel() {
const { showPanelHeader = true } = this.props;
private renderPanels() {
const { renderCurrentPanelOnly = true } = this.props;
const { stack } = this.state;
if (stack.length === 0) {
return null;
}
const [activePanel, previousPanel] = stack;
const panelsToRender = renderCurrentPanelOnly ? [stack[0]] : stack;
const panelViews = panelsToRender.map(this.renderPanel).reverse();
return panelViews;
}

private renderPanel = (panel: IPanel, index: number) => {
const { showPanelHeader = true } = this.props;
const { stack } = this.state;
const active = index === 0;
const layer = stack.length - index;
const key = `${layer}-${active}`;
Copy link
Contributor Author

@radekmie radekmie Oct 6, 2019

Choose a reason for hiding this comment

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

The active part is required for CSSTransition to properly animate the bottom panel to move in while popping and moving out while pushing. It changes the key though, so the PanelView is rerendered anyway, so no state is preserved. Removing it makes it work, the state is preserved, but the bottom panel is not animated at all.

I'm not sure how to tackle this issue in a good way. A somewhat hacky may include an additional field in the state to remember the active index (last fired onEntered) and set the class manually.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm confused, what does key have to do with CSSTransition? I don't see it in the docs http://reactcommunity.org/react-transition-group/css-transition

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The thing is that with renderCurrentPanelOnly={false} we would keep all the CSSTransitions rendered, therefore they would not trigger the enter event as they were entered. To force the enter event, I wanted to change the key (just like the current implementation), but stack.length is no longer OK, as the length changes with the, well, stack length, and a single panel should not rerender as long as it's hidden.

This key contains two parts: first one, stack.length - index is constant (and unique) for each panel, second one, active changes only when the panel becomes or stops being active.

return (
<CSSTransition classNames={Classes.PANEL_STACK} key={stack.length} timeout={400}>
<CSSTransition classNames={Classes.PANEL_STACK} key={key} timeout={400}>
<PanelView
onClose={this.handlePanelClose}
onOpen={this.handlePanelOpen}
panel={activePanel}
previousPanel={previousPanel}
panel={panel}
previousPanel={stack[index + 1]}
showHeader={showPanelHeader}
/>
</CSSTransition>
);
}
};

private handlePanelClose = (panel: IPanel) => {
const { stack } = this.state;
Expand Down
21 changes: 21 additions & 0 deletions packages/core/test/panel-stack/panelStackTests.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -234,6 +234,27 @@ describe("<PanelStack>", () => {
assert.equal(firstPanelHeader.at(0).text(), "Test Title");
});

it("renders only one panel by default", () => {
const stack = [{ component: TestPanel, title: "Panel A" }, { component: TestPanel, title: "Panel B" }];
panelStackWrapper = renderPanelStack({ stack });

const panelHeaders = panelStackWrapper.findClass(Classes.HEADING);
assert.exists(panelHeaders);
assert.equal(panelHeaders.length, 1);
assert.equal(panelHeaders.at(0).text(), stack[1].title);
});

it("renders all panels with renderCurrentPanelOnly disabled", () => {
const stack = [{ component: TestPanel, title: "Panel A" }, { component: TestPanel, title: "Panel B" }];
panelStackWrapper = renderPanelStack({ renderCurrentPanelOnly: false, stack });

const panelHeaders = panelStackWrapper.findClass(Classes.HEADING);
assert.exists(panelHeaders);
assert.equal(panelHeaders.length, 2);
assert.equal(panelHeaders.at(0).text(), stack[0].title);
assert.equal(panelHeaders.at(1).text(), stack[1].title);
});

interface IPanelStackWrapper extends ReactWrapper<IPanelStackProps, any> {
findClass(className: string): ReactWrapper<React.HTMLAttributes<HTMLElement>, any>;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import { Button, H5, Intent, IPanel, IPanelProps, PanelStack, Switch, UL } from
import { Example, handleBooleanChange, IExampleProps } from "@blueprintjs/docs-theme";

export interface IPanelStackExampleState {
activePanelOnly: boolean;
currentPanelStack: IPanel[];
showHeader: boolean;
}
Expand All @@ -34,15 +35,22 @@ export class PanelStackExample extends React.PureComponent<IExampleProps, IPanel
};

public state = {
activePanelOnly: true,
currentPanelStack: [this.initialPanel],
showHeader: true,
};

private toggleActiveOnly = handleBooleanChange((activePanelOnly: boolean) => this.setState({ activePanelOnly }));
private handleHeaderChange = handleBooleanChange((showHeader: boolean) => this.setState({ showHeader }));

public render() {
const stackList = (
<>
<Switch
checked={this.state.activePanelOnly}
label="Render active panel only"
onChange={this.toggleActiveOnly}
/>
<Switch checked={this.state.showHeader} label="Show panel header" onChange={this.handleHeaderChange} />
<H5>Current stack</H5>
<UL>
Expand All @@ -59,6 +67,7 @@ export class PanelStackExample extends React.PureComponent<IExampleProps, IPanel
initialPanel={this.state.currentPanelStack[0]}
onOpen={this.addToPanelStack}
onClose={this.removeFromPanelStack}
renderCurrentPanelOnly={this.state.activePanelOnly}
showPanelHeader={this.state.showHeader}
/>
</Example>
Expand Down