[Core] Add panel stack component #2642

kadhirvelm · 2018-07-05T17:41:44Z

Bugs

There's a strange bug that if you smash the Open new panel button rapidly, it spits out the panels incorrectly in the onOpen callback handler.

The panel stack isn't really meant to be used like that, but either way I don't think the component should be merged with a bug like that. Wanted to get the PR up in the meantime and get some feedback while I continue to figure this out.

Checklist

Include tests
Update documentation

Changes proposed in this pull request:

Adds the panel stack component to blueprint core:

Reviewers should focus on:

The core component was already reviewed, so best to focus on the example, documentation, and that overall the component was properly integrated into blueprint.

Also I've been having issues getting the transition to work correctly, so any thoughts on how to fix that nicely would be very appreciated.

GIF

palantirtech · 2018-07-05T17:41:45Z

Thanks for your interest in palantir/blueprint, @kadhirvelm! Before we can accept your pull request, you need to sign our contributor license agreement - just visit https://cla.palantir.com/ and follow the instructions. Once you sign, I'll automatically update this pull request.

blueprint-bot · 2018-07-05T17:51:10Z

Made lower case

Preview: documentation | landing | table

giladgray

needs dark theme and style love

giladgray · 2018-07-05T18:11:39Z

packages/core/src/components/panel-stack/panel-stack.md

+`Panel Stack` is a component designed to render multiple panels from a stack, but only display the
+top most panel at any given time.
+
+It will render each panel with a header and uses [`CSSTransitionGroup`](https://facebook.github.io/react/docs/animation.html)


it's just CSSTransition now and link to docs directly http://reactcommunity.org/react-transition-group/css-transition

giladgray · 2018-07-05T19:10:32Z

packages/core/src/components/panel-stack/panel-stack.md

+@## Props
+
+The `Panel Stack` component is available in the __@blueprintjs/core__ package.
+Make sure to review the [general usage docs for JS components](#blueprint.usage).


remove this paragraph, we don't do this anymore.

giladgray · 2018-07-05T19:11:12Z

packages/core/src/components/panel-stack/panel-stack.scss

+  position: relative;
+  overflow: hidden;
+
+  .push-#{$ns}-panel-stack-transition-enter {


$ns must be the first thing in the class. #{$ns}-panel-stack-transition-push-*

giladgray · 2018-07-05T19:24:57Z

packages/core/src/components/panel-stack/panel-stack.scss

+    transform: translateX(100%);
+  }
+
+  .push-#{$ns}-panel-stack-transition-enter-active {


TODO: use the mixin

giladgray · 2018-07-05T19:26:38Z

packages/core/src/components/panel-stack/panelStack.tsx

+
+export interface IPanelStackProps extends IProps {
+    /** The panel to show when initially rendering the component */
+    initialPanel: IPanel;


must this be required?

@giladgray I think it must. The only way to open a panel (after the initial panel becomes the stack) is from a panel. If there are no more panels, the stack will stay empty, forever. I don't think there's a valid use case for that.

oh yeah that makes a lot of sense. probably worth calling that out in some usage docs.

giladgray · 2018-07-05T19:30:42Z

packages/core/src/components/panel-stack/panel-stack.scss

+    margin: 0 10px;
+    // aligns title with back button
+    line-height: 28px;
+    color: $pt-text-color;


this is the default, why must it be reset? if so, needs dark theme

giladgray · 2018-07-05T19:32:22Z

packages/core/src/components/panel-stack/panel-stack.scss

+    }
+
+    .#{$ns}-panel-stack-chevron {
+      font-size: $pt-font-size;


font-size has no effect on svg icons. please remove.

giladgray · 2018-07-05T19:32:42Z

packages/core/src/components/panel-stack/panel-stack.scss

+  right: 0;
+  bottom: 0;
+  left: 0;
+  background-color: $white;


needs dark theme

giladgray · 2018-07-05T19:32:53Z

packages/core/src/components/panel-stack/panel-stack.scss

+  top: 0;
+  right: 0;
+  bottom: 0;
+  left: 0;


@include position-all(absolute, 0)

giladgray · 2018-07-05T19:33:19Z

packages/core/src/components/panel-stack/panelHeader.tsx

+            return null;
+        }
+        return (
+            <div className={Classes.PANELSTACK_HEADER_BACK} onClick={this.props.back.onClick}>


🎨 i feel like we should just use a small minimal button here instead of all this custom style

Tried to remove whatever I could, now it's just flex + cursor + color

kadhirvelm · 2018-07-10T18:45:59Z

kadhirvelm · 2018-07-10T18:47:14Z

So I tried to add the react-transition-phase mixin, but I'm definitely not using it right. Could use some help.

But on the plus side, changing the duration to pt-transition-duration fixed the bug mentioned above (the onOpen callback receiving the wrong panel)!

blueprint-bot · 2018-07-10T18:51:33Z

Dark theme added

Preview: documentation | landing | table

blueprint-bot · 2018-07-10T18:56:57Z

Linting fixed

Preview: documentation | landing | table

giladgray · 2018-07-23T16:21:42Z

packages/core/src/components/panel-stack/panelStack.tsx

+export class PanelStack extends React.PureComponent<IPanelStackProps, IPanelStackState> {
+    public constructor(props: IPanelStackProps, context?: any) {
+        super(props, context);
+        this.state = {


pull this out to member, remove constructor. you can access this.props in member definitions.

giladgray · 2018-07-23T16:23:47Z

packages/core/src/components/panel-stack/panelStack.tsx

+                timeout={{
+                    enter: 400,
+                    exit: 400,
+                }}


timeout={400}?

giladgray · 2018-07-23T16:24:53Z

packages/core/src/components/panel-stack/panelStack.tsx

+        return (
+            <CSSTransition
+                classNames={`${Classes.PANELSTACK_TRANSITION}-${this.state.direction}`}
+                key={stack.length}


this does not feel safe to use as a key. the size of the stack is not a unique identifier of the panel. not really sure what to do tho, needs investigation.

giladgray · 2018-07-23T16:25:13Z

packages/core/src/components/panel-stack/panelStack.tsx

+                }}
+            >
+                <PanelView
+                    key={stack.length}


no need for key here, it's not in an array.

not sure about transition group 2.x, but in 1.x the key was required for their bookkeeping, not react's. Maybe it needs to be on the transition now?

giladgray · 2018-07-23T16:25:54Z

packages/core/src/components/panel-stack/panelStack.tsx

+                // Prevent the last panel from being closed.
+                state.stack.length > 1;
+            if (allowed && this.props.onClose !== undefined) {
+                this.props.onClose(state.stack[0]);


Utils.safeInvoke instead of undefined check

giladgray · 2018-07-23T16:29:56Z

packages/core/src/components/panel-stack/panelHeader.tsx

+}
+
+export interface IPanelHeaderProps {
+    back?: IBackButton;


prefer to avoid objects as props as it often breaks the pure behavior by creating new objects every render (you do this below). spell them out as backTitle and onBackClick.

giladgray · 2018-07-23T16:30:28Z

packages/core/src/components/panel-stack/panelHeader.tsx

+
+export interface IPanelHeaderProps {
+    back?: IBackButton;
+    title: string;


why not use children for this? <PanelHeader>content</PanelHeader>

invliD · 2018-07-23T16:58:34Z

packages/core/src/components/panel-stack/panelStack.tsx

+                }}
+            >
+                <PanelView
+                    key={stack.length}


not sure about transition group 2.x, but in 1.x the key was required for their bookkeeping, not react's. Maybe it needs to be on the transition now?

invliD · 2018-07-23T17:00:00Z

packages/core/src/components/panel-stack/panelStack.tsx

+                // otherwise rapidly clicking the back link will close multiple panels.
+                state.stack.length === stackSize &&
+                // Prevent the last panel from being closed.
+                state.stack.length > 1;


this can be refactored to just if (condition) { return null }
I initially wrote it this way because I had problems with setState typings, but returning null should work and allows you to remove the allowed variable entirely.

kadhirvelm · 2018-07-23T20:50:09Z

packages/core/src/components/panel-stack/panelHeader.tsx

+     * The name of the previous panel header in the stack. Will not render unless an onBackClick
+     * handler is defined.
+     */
+    backTitle?: string;


Oh hum, should this ever be undefined? title

Should we allow a panel to be defined without a title?

blueprint-bot · 2018-07-23T20:52:25Z

Review items fixed, transitions reverted

Preview: documentation | landing | table

giladgray · 2018-07-23T23:06:03Z

packages/core/src/components/panel-stack/panelStack.tsx

+            >
+                <PanelView
+                    key={stack.length}
+                    panelClassName={this.props.className}


@kadhirvelm why is this prop passed down here? it gets rendered on two elements, which seems undesirable.

…-stack

blueprint-bot · 2018-07-23T23:43:32Z

Removed extra panelClassName

Preview: documentation | landing | table

blueprint-bot · 2018-07-23T23:45:37Z

Merge branch 'develop' of github.com:palantir/blueprint into km/panel-stack

Preview: documentation | landing | table

blueprint-bot · 2018-07-23T23:48:27Z

minor docs

Preview: documentation | landing | table

blueprint-bot · 2018-07-24T01:05:51Z

Adjusted transition to include opacity and -50% left movement

Preview: documentation | landing | table

blueprint-bot · 2018-07-24T01:15:11Z

Removed useless type checking

Preview: documentation | landing | table

blueprint-bot · 2018-07-24T01:26:21Z

Fixed opacity

Preview: documentation | landing | table

blueprint-bot · 2018-07-24T01:26:57Z

Opacity

Preview: documentation | landing | table

blueprint-bot · 2018-07-24T01:36:17Z

Removed 0

Preview: documentation | landing | table

blueprint-bot · 2018-07-24T01:49:46Z

Reversed opacities

Preview: documentation | landing | table

blueprint-bot · 2018-07-24T01:56:35Z

One more attempt

Preview: documentation | landing | table

* merge PanelHeader into PanelView * panelProps file with more docs * panelProps.ts * export panelProps, more docs * example key * docs * safeInvoke

blueprint-bot · 2018-07-24T18:27:22Z

[PanelStack] merge some files (#2719)

Preview: documentation | landing | table

blueprint-bot · 2018-07-26T14:44:54Z

changed easing settings

Preview: documentation | landing | table

blueprint-bot · 2018-07-30T20:45:43Z

onClose/Open param names

Preview: documentation | landing | table

* openPanel accepts IPanel instead of three args * PanelView onClose/Open props skips getPanelProps() step * fix tests * remove IPanelPptions

blueprint-bot · 2018-07-30T21:55:47Z

[PanelStack] use IPanel in openPanel() public API (#2738)

Preview: documentation | landing | table

* PanelStack docs * more docs refactors * adjust docs-modifiers margins * remove "pop the stack"

blueprint-bot · 2018-07-30T22:41:40Z

[PanelStack] documentation (#2737)

Preview: documentation | landing | table

Kadhirvel Manickam added 5 commits July 2, 2018 18:33

Added panel stack

7b4d4d1

First test integrated

f781b5a

Tests added

5c106b5

Transitions adjusted

22ac407

Updated readme

dce1189

kadhirvelm requested a review from giladgray July 5, 2018 17:42

Made lower case

f7c207a

giladgray suggested changes Jul 5, 2018

View reviewed changes

Dark theme added

e0f7728

Linting fixed

b637e94

giladgray suggested changes Jul 23, 2018

View reviewed changes

invliD reviewed Jul 23, 2018

View reviewed changes

Review items fixed, transitions reverted

7da109d

kadhirvelm commented Jul 23, 2018

View reviewed changes

giladgray reviewed Jul 23, 2018

View reviewed changes

Kadhirvel Manickam and others added 2 commits July 23, 2018 16:37

Removed extra panelClassName

a128368

Merge branch 'develop' of github.com:palantir/blueprint into km/panel…

49e7845

…-stack

Gilad Gray added 2 commits July 23, 2018 16:43

_panel-stack.scss

7c6a914

minor docs

8fbe610

updated tests

82bcdee

Adjusted transition to include opacity and -50% left movement

4faf1c6

Removed useless type checking

a4eb098

Kadhirvel Manickam added 2 commits July 23, 2018 18:21

Fixed opacity

2a7a459

Opacity

7f8f6e1

Kadhirvel Manickam added 2 commits July 23, 2018 18:28

Opacity flipped

223c310

Removed 0

634e211

Reversed opacities

0246e3f

One more attempt

b0e19c0

[PanelStack] merge some files (#2719)

6088348

* merge PanelHeader into PanelView * panelProps file with more docs * panelProps.ts * export panelProps, more docs * example key * docs * safeInvoke

giladgray requested a review from pkwi July 25, 2018 17:12

changed easing settings

0fb825d

Gilad Gray added 2 commits July 30, 2018 13:37

fix copyrights

c6acec0

onClose/Open param names

fa37554

[PanelStack] use IPanel in openPanel() public API (#2738)

9dac016

* openPanel accepts IPanel instead of three args * PanelView onClose/Open props skips getPanelProps() step * fix tests * remove IPanelPptions

[PanelStack] documentation (#2737)

be7d617

* PanelStack docs * more docs refactors * adjust docs-modifiers margins * remove "pop the stack"

giladgray approved these changes Jul 30, 2018

View reviewed changes

giladgray merged commit 6581f83 into develop Jul 30, 2018

giladgray deleted the km/panel-stack branch July 30, 2018 23:29

[Core] Add panel stack component #2642

[Core] Add panel stack component #2642

Conversation

kadhirvelm commented Jul 5, 2018

Bugs

Checklist

Changes proposed in this pull request:

Reviewers should focus on:

GIF

palantirtech commented Jul 5, 2018

blueprint-bot commented Jul 5, 2018

Made lower case

giladgray left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kadhirvelm commented Jul 10, 2018

kadhirvelm commented Jul 10, 2018 • edited Loading

blueprint-bot commented Jul 10, 2018

Dark theme added

blueprint-bot commented Jul 10, 2018

Linting fixed

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blueprint-bot commented Jul 23, 2018

Review items fixed, transitions reverted

Choose a reason for hiding this comment

blueprint-bot commented Jul 23, 2018

Removed extra panelClassName

blueprint-bot commented Jul 23, 2018

Merge branch 'develop' of github.com:palantir/blueprint into km/panel-stack

blueprint-bot commented Jul 23, 2018

minor docs

blueprint-bot commented Jul 24, 2018

Adjusted transition to include opacity and -50% left movement

blueprint-bot commented Jul 24, 2018

Removed useless type checking

blueprint-bot commented Jul 24, 2018

Fixed opacity

blueprint-bot commented Jul 24, 2018

Opacity

blueprint-bot commented Jul 24, 2018

Removed 0

blueprint-bot commented Jul 24, 2018

Reversed opacities

blueprint-bot commented Jul 24, 2018

One more attempt

blueprint-bot commented Jul 24, 2018

[PanelStack] merge some files (#2719)

blueprint-bot commented Jul 26, 2018

changed easing settings

blueprint-bot commented Jul 30, 2018

onClose/Open param names

blueprint-bot commented Jul 30, 2018

[PanelStack] use IPanel in openPanel() public API (#2738)

blueprint-bot commented Jul 30, 2018

[PanelStack] documentation (#2737)

kadhirvelm commented Jul 10, 2018 •

edited

Loading