Components: Add generic panel components to build the sidebar elements #789

youknowriad · 2017-05-15T09:24:55Z

This PR creates generic container components to build the sidebar panels

<Panel>
	<PanelHeader>
		// Panel Header content
	</PanelHeader>
	<PanelBody>
		// Panel Body content
	</PanelBody>
</Panel>

jasmussen · 2017-05-15T09:57:01Z

I love it! I love that it's something you're working on. Thank you!

There's not a ton to see yet, correct?

Markup looks good. Only one panel migth use different markup as far as I can imagine right now, revisions:

And even that panel can launch with less.

youknowriad · 2017-05-15T10:20:41Z

@jasmussen Right! There's not so much right now. It just adds some visual components, to be used to build the different panels. I'll be adding some "non-fonctional" UI elements before merging this.

And I'll work separately on the first toggle maybe.

youknowriad · 2017-05-15T10:29:21Z

@jasmussen Can you take another look? I've added a generic component to open/close the panel body.

jasmussen · 2017-05-15T11:28:40Z

Can you take another look? I've added a generic component to open/close the panel body.

Love it. Works exactly like I hoped it would. I'll add a few style changes at some point, and we'll still need the #450 ingredient in order for it all to come together. But that shouldn't be a blocker for this.

youknowriad · 2017-05-15T11:47:26Z

I've pushed this a bit forward, adding a generic FormToggle component (shamelessly copied from Calypso). Still not functional, but this is another UI building block.

aduth · 2017-05-15T13:05:45Z

I'm wondering if we could simplify the component to accept a header label as a prop, so that it can be rendered as as a single component instead of three:

<Panel header={ __( 'Post Settings' ) }>
	<PostStatus />
</Panel>

But this would require that controls are consistent across panels, or customized with a separate prop (seems fine if it's a rarer case).

jasmussen · 2017-05-15T13:08:45Z

In Calypso there's sometimes a little "summary" note when the panel is collapsed:

I don't necessarily think we want this in the core editor, initially as it's simply extra complexity. Also, if you can re-sort and hide panels, as well as have their open/closed state remembered (which we want for core but which Calypso does't do), then the added value is arguably minimal.

youknowriad · 2017-05-15T13:12:37Z

I'm wondering if we could simplify the component to accept a header label as a prop

@aduth I've always struggled to use this component in Calypso (SectionHeader) because of this enforced "shape", that's why I prefer the flexibility of a PanelHeader container component.

In the current mockups we have two panels. One with icons and one without icons. But I prefer to look at these panels as panels that could be reused in other parts of WP, that's why I opted for the most flexible approach.

In Calypso there's sometimes a little "summary" note when the panel is collapsed

@jasmussen Seems like an easy addition to the PostBodyToggle component when necessary.

youknowriad · 2017-05-15T13:16:07Z

@aduth I think we could have a compromise where if we provide a header prop to the Panel component, the Panel component could include the PanelHeader and in the same time, having the possibility to avoid this prop and provide a PanelHeader explicitely.

Edit: Prop added in aaf06c2

aduth · 2017-05-15T13:08:25Z

editor/sidebar/post-status/index.js

+	render() {
+		return (
+			<PanelBody>
+				<PanelBodyToggle


(I'm unsure:) Do you think it'd make any more sense to render this as a component which accepts children and manages "open" state internally?

<PanelToggleSection initialOpen={ true }> <div className="editor-sidebar-post-status__row"></div> </PanelToggleSection>

This makes me think we should add this behavior to the PanelBody instead, what do you think?

<PanelBody toggle={ __( 'Status & Visibility' ) } initialOpen={ true }> <div className="editor-sidebar-post-status__row"></div> </PanelBody>

Sure, that seems fine. It's worked for us elsewhere as a generic Accordion component with initialExpanded, title (and per previous note, subtitle).

https://github.com/Automattic/wp-calypso/blob/f19b119/client/components/accordion/index.jsx#L16-L22

aduth · 2017-05-15T13:08:36Z

editor/sidebar/post-status/index.js

+					label={ __( 'Status & Visibility' ) }
+				/>
+				{ this.state.opened &&
+					<div>


Can we drop the extra wrapper?

no, we'll have a lot of rows later. I've just added one for now as an example.

no, we'll have a lot of rows later. I've just added one for now as an example.

I still don't see why even if we had many rows we'd need an empty wrapper.

Sorry, I've overlooked here. It was necessary when the this.state.opened check was in this file, but it's not anymore.

aduth · 2017-05-15T13:09:15Z

editor/sidebar/post-status/index.js

+							<span>{ __( 'Pending review' ) }</span>
+							<FormToggle
+								checked={ false }
+								onChange={ () => {} }


Maybe we ought to define a defaultProps for this handler on FormToggle so it doesn't throw an error when not defined.

I think this would be always defined since this is just a non-functional example. I've added this.

I'm on the fence. We're not using it anywhere and I'm not sure about the future of this feature in React.

I'm on the fence. We're not using it anywhere and I'm not sure about the future of this feature in React.

Which feature? defaultProps? If it were to be deprecated, there'd still be some mechanism for defining defaults, likely through destructuring:

const { onChange = noop } = this.props;

I note here because it seems odd to me we'd prefer to have an error thrown when the handler isn't defined.

aduth · 2017-05-15T13:11:37Z

components/panel-body/style.scss

@@ -0,0 +1,4 @@
+.components-panel-body {
+	padding: 15px;
+	box-shadow: 0 0 0 1px $light-gray-500;


At a glance, could we use border here instead? I'm sensitive to become overly reliant on box-shadow as it can have miserable performance.

http://tobiasahlin.com/blog/how-to-animate-box-shadow/
https://medium.com/airbnb-engineering/css-box-shadow-can-slow-down-scrolling-d8ea47ec6867

A box-shadow is more "generic" because we could use several panels one after another and having only one line (the borders collapse) while this is not the case with border.

Couldn't we recreate that with .panel + .panel { margin-top: -1px; } ?

yep but not as easy, I'd like the PanelBody, PanelHeader and Panel to be used together (multiple headers, multiple bodies) and in the same time we could use them separately. The three components would have this border and two of them could be nested inside the Panel component.

I think your proposal is possible but we'd probably have a lot of use-cases. I'll try it

aduth · 2017-05-15T13:12:23Z

components/form-toggle/style.scss

+}
+
+.components-form-toggle {
+	.accessible-focus &:focus {


I don't think .accessible-focus exists.

aduth · 2017-05-15T13:13:14Z

components/form-toggle/index.js

+	}
+}
+
+FormToggle.idNum = 1;


Elsewhere we're using instances instead of idNum. I don't have a strong preference either way (aside maybe avoiding the abbreviation), but we should be consistent.

https://github.com/WordPress/gutenberg/blob/3c2db1d/editor/inserter/menu.js#L287

aduth · 2017-05-15T13:14:09Z

components/form-toggle/index.js

+	}
+
+	render() {
+		const id = this.props.id || 'toggle-' + this.id;


My YAGNI senses are tingling on the yet-unused id prop.

aduth · 2017-05-15T13:45:01Z

components/form-toggle/index.js

+					disabled={ this.props.disabled }
+					/>
+				<label className="components-form-toggle__label" htmlFor={ id } >
+					<span className="components-form-toggle__switch"


(Acknowledging it's sourced externally) this implementation seems overly complicated to me. I think it could be simplified if we allowed the checkbox to behave normally but hidden, then simply style the label based on the current state of the checkbox.

Example: https://codepen.io/aduth/pen/wdXmwK

For now, I'm going to leave this as is, there are some accessibility concerns behind this implementation I think. The span here is playing the role of the hidden input.

Why would we leave this as-is? Just because it's copied from an external source doesn't mean it shouldn't be subject to the same scrutiny as any other code? So far as accessibility is concerned, my proposed simplification handles this better by not even bothering to try to create a fake input, but rather letting the true underlying input handle the intended change behaviors.

Why would we leave this as-is? Just because it's copied from an external source doesn't mean it shouldn't be subject to the same scrutiny as any other code?

I totally agree

So far as accessibility is concerned, my proposed simplification handles this better by not even bothering to try to create a fake input, but rather letting the true underlying input handle the intended change behaviors.

I'm not very familiar with accessibility concerns but I thought that since we're hiding the input, we should provide a "replacement" simulating its behaviour. I can update the code if it's not case.

I'm not very familiar with accessibility concerns but I thought that since we're hiding the input, we should provide a "replacement" simulating its behaviour.

In my demo, it's more that the input is visually hidden, but still present on the page so that it can receive and react to focus and keyboard events natively. In the Calypso implementation, this was recreated on a static element with a set of ARIA and role props, tabIndex for focus, and keyboard/click handlers. I raise primarily because it seems to me like needless and fragile complexity.

aduth · 2017-05-15T13:45:51Z

components/form-toggle/index.js

+			return;
+		}
+
+		if ( event.key === 'Enter' || event.key === ' ' ) {


key is slightly outside acceptable browser support (only available in latest versions of Safari)

http://caniuse.com/#feat=keyboardevent-key

I confess I haven't took a deep look to the FormToggle component stolen from Calypso

aduth · 2017-05-15T13:49:50Z

If we kept the separate components, could we at least consolidate them into the components/panel directory instead of three separate directories? PanelHeader and PanelBody don't seem to serve a standalone purpose justifying the separation.

mtias · 2017-05-15T18:31:19Z

components/panel/style.scss

+	background: $white;
+	border: 1px solid $light-gray-500;
+
+	> .components-panel__header,


Why do we need the descendant selectors here?

Technically we could have panels inside other panels, so to avoid applying the margin to the sub-panels, I've preferred to restrict it to the direct descendant only. It's safer IMO.

Edit: Or maybe you're proposing to avoid nesting at all? This allows the header and body to be usable outside of panels and as panel descendants as well.

I don't think we should optimize for reusing the panel pieces, only the panels themselves at this stage. If something is reusuable outside a panel, then it should be a separate component and get its own class for clarity.

Ok! moving those styles

youknowriad · 2017-05-16T15:04:02Z

Can I have a final look here?

aduth · 2017-05-16T15:47:48Z

components/form-toggle/style.scss

@@ -0,0 +1,55 @@
+.components-form-toggle {
+  position: relative;


Mixed whitespace here and in the selector below.

aduth · 2017-05-16T15:48:48Z

components/panel/body-toggle.js

+
+function PanelBodyToggle( { opened, label, onClick } ) {
+	return (
+		<button className="components-panel__body-toggle" onClick={ onClick }>


Out of curiosity, how difficult would it be to render and style this as an IconButton and avoid the need for a specialized PanelBodyToggle altogether?

Regardless, we should do one of either use Button as a base (for consistency) or add type="button" to avoid this acting like a submit if ever used in a form.

Not that difficult but my thinking here is that this component could be used as standalone. But maybe I'm optimising for unnecessary features.

aduth · 2017-05-16T15:50:24Z

components/panel/body.js

+		this.setState( {
+			opened: ! this.state.opened,
+		} );
+	}


Minor: For consistency, should have a blank line between toggle and render.

aduth · 2017-05-16T15:51:17Z

components/panel/body.js

+	}
+
+	toggle( event ) {
+		event.preventDefault();


Ah, I guess this would prevent the form submission anyways. Still might be good to be intentional with the type prop on button.

aduth · 2017-05-16T15:52:16Z

components/panel/body.js

+		const { toggle, children } = this.props;
+		return (
+			<div className="components-panel__body">
+				{ !! toggle &&


At a glance it wasn't clear to me what toggle represented. Found later it's the label. Should we call it something more along those lines? label, title, toggleLabel, heading?

aduth · 2017-05-16T15:53:54Z

components/panel/header.js

@@ -0,0 +1,9 @@
+function PanelHeader( { children } ) {
+	return (
+		<div className="components-panel__header">


Seems odd we push responsibility of wrapping strong to the rendering component instead of baking in consistent behavior to the PanelHeader itself. I see we have a need to exempt icons from bold effect, but maybe this could be achieved with a separate prop instead?

aduth · 2017-05-16T16:03:58Z

editor/sidebar/post-status/index.js

+			<div>
+				<div className="editor-sidebar-post-status__row">
+					<span>{ __( 'Pending review' ) }</span>
+					<FormToggle />


This causes a warning to be logged about button as descendant of button (<FormToggle> of <PanelBodyToggle>)

react-dom.development.js:498 Warning: validateDOMNesting(...): <button> cannot appear as a descendant of <button>. in button (created by Button) in Button (created by IconButton) in IconButton (created by PanelBodyToggle) in button (created by PanelBodyToggle) in PanelBodyToggle (created by PanelBody) in div (created by PanelBody) in PanelBody (created by PostStatus) in PostStatus (created by PostSettings) in div (created by Panel) in Panel (created by PostSettings) in PostSettings (created by Connect(PostSettings)) in Connect(PostSettings) (created by Sidebar) in div (created by Sidebar) in Sidebar (created by Layout) in div (created by Layout) in Layout (created by Connect(Layout)) in Connect(Layout) in Provider in Provider

I'm finding buttons to be rather difficult to work with due to restrictions on the types of of elements they accept as children.

Good catch, with my docker setup, I was thinking I'm in debug mode by default, but it wasn't the case.

youknowriad · 2017-05-16T16:33:38Z

Tks for the review @aduth I've addressed the raised points.

aduth

This generally looks good 👍 Left a few minor notes.

aduth · 2017-05-16T21:13:26Z

editor/sidebar/index.js

+			<PostSettings />
+			<Panel>
+				<PanelHeader>
+					<strong>


Should we use the new label prop, or did you have challenges with the custom className? I'd expect label={ <span ... /> } should work well enough.

aduth · 2017-05-16T21:15:01Z

editor/sidebar/post-status/index.js

+function PostStatus() {
+	return (
+		<PanelBody title={ __( 'Status & Visibility' ) }>
+			<div className="editor-sidebar-post-status__row">


By className conventions we'd intentionally avoid reflecting nesting in the class name to allow the component to be more easily moved if necessary. I think this ought to be simplified to editor-post-status__row. Else maybe rename the parent directory to sidebar-post-status.

…assName

youknowriad added the Design label May 15, 2017

youknowriad self-assigned this May 15, 2017

youknowriad requested a review from jasmussen May 15, 2017 09:24

youknowriad force-pushed the add/panel-components branch from be861f6 to 5222e32 Compare May 15, 2017 11:52

youknowriad force-pushed the add/panel-components branch from 5222e32 to 3c445d5 Compare May 15, 2017 13:07

youknowriad force-pushed the add/panel-components branch from 3c445d5 to 1c64eab Compare May 15, 2017 13:23

aduth reviewed May 15, 2017

View reviewed changes

mtias reviewed May 15, 2017

View reviewed changes

youknowriad added 12 commits May 16, 2017 09:32

Components: Add generic panel components to build the sidebar elements

17f14f1

Post Settings: Extract The Post Settings panel to its own component

edf72bc

Components: Adding a generic panel body toggle

cada1b6

Styling: Fix style indentation

450e5ed

Components: Adding the FormToggle component

abfaf37

Components: Adding an option header prop to the Panel component

39b6b31

Components: Gather all panel-* components in the panel directory

42bb71f

FormToggle: Default Props and drop unused props

c72eb1e

PanelBody: Adding the possibility to toggle the content of PanelBody

b042858

Panels: Using borders instead of box-shadow

996d2e3

Form Toggle: Simplify the component. props @aduth

416e523

Sidebar: Fix panels after rebase

5c090ff

youknowriad force-pushed the add/panel-components branch from 75a27c3 to 5c090ff Compare May 16, 2017 08:40

Panel: Avoid nesting styles

ae9a433

aduth reviewed May 16, 2017

View reviewed changes

youknowriad added 3 commits May 16, 2017 17:19

FormToggle: Fix style indent

5636762

Panel: Dropping the BodyToggle component

f3bf935

Panel: Rename toggle prop

7633875

youknowriad force-pushed the add/panel-components branch from 3a53bb6 to 7633875 Compare May 16, 2017 16:28

Panel Header: Adding a label prop

a394bbd

aduth approved these changes May 16, 2017

View reviewed changes

PostStatus: Dropping unnecessary wrapper, using prop and fix wrong cl…

9d33716

…assName

youknowriad force-pushed the add/panel-components branch from c37c83e to 9d33716 Compare May 17, 2017 08:05

youknowriad merged commit a67c9bb into master May 17, 2017

youknowriad deleted the add/panel-components branch May 17, 2017 08:09

		@@ -0,0 +1,55 @@
		.components-form-toggle {
		position: relative;

Components: Add generic panel components to build the sidebar elements #789

Components: Add generic panel components to build the sidebar elements #789

Conversation

youknowriad commented May 15, 2017

jasmussen commented May 15, 2017

youknowriad commented May 15, 2017

youknowriad commented May 15, 2017

jasmussen commented May 15, 2017

youknowriad commented May 15, 2017 • edited Loading

aduth commented May 15, 2017

jasmussen commented May 15, 2017

youknowriad commented May 15, 2017 • edited Loading

youknowriad commented May 15, 2017 • edited Loading

aduth May 15, 2017 • edited Loading

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

youknowriad May 15, 2017 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aduth commented May 15, 2017

Choose a reason for hiding this comment

youknowriad May 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youknowriad commented May 16, 2017

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

youknowriad commented May 16, 2017

aduth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youknowriad commented May 15, 2017 •

edited

Loading

youknowriad commented May 15, 2017 •

edited

Loading

youknowriad commented May 15, 2017 •

edited

Loading

aduth May 15, 2017 •

edited

Loading

youknowriad May 15, 2017 •

edited

Loading

youknowriad May 16, 2017 •

edited

Loading