TitlePane #26

msssk · 2017-02-08T21:09:13Z

Type: bug / feature

The following has been addressed in the PR:

There is a related issue
All code matches the style guide
Unit or Functional tests are included in the PR

Description:

This PR introduces a TitlePane component very similar to Dijit's TitlePane.

A title pane should be a suitable child for an Accordion widget.

codecov-io · 2017-02-08T21:18:57Z

Codecov Report

Merging #26 into master will decrease coverage by 0.06%.
The diff coverage is 97.82%.

@@            Coverage Diff             @@
##           master      #26      +/-   ##
==========================================
- Coverage   99.24%   99.17%   -0.07%     
==========================================
  Files          18       19       +1     
  Lines         928      972      +44     
  Branches      279      295      +16     
==========================================
+ Hits          921      964      +43     
  Misses          1        1              
- Partials        6        7       +1

Impacted Files	Coverage Δ
src/main.ts	`100% <100%> (ø)`	⬆️
src/titlepane/TitlePane.ts	`97.61% <97.61%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2259bf6...46b873e. Read the comment docs.

bitpshr · 2017-02-08T22:10:04Z

src/components/titlePane/createTitlePane.ts

+};
+
+export type TitlePane = Widget<TitlePaneProperties> & ThemeableMixin & {
+	onClickTitle?(): void;


Event handlers shouldn't be optional since you're the widget author and you know they'll always be present.

Thanks, fixed!

bitpshr · 2017-02-08T22:10:48Z

src/components/titlePane/createTitlePane.ts

+	collapsible?: boolean;
+	enterAnimation?: string;
+	exitAnimation?: string;
+	title: string;


title should probably be an optional property like it is on the Dialog. It looks like you're already correctly defaulting it to an empty string in getChildrenNodes.

Should it? I feel like if you haven't specified title, you've chosen the wrong widget.

Fair. In that case, there's no need to default it when you destructure in getChildrenNodes.

Thanks - I tried just now but v() doesn't accept undefined as a child, so we either have to cast to string when passing it as the child to v('div', ..., [ String(title) ]) or default it... I'm a little preferring "default it".

bitpshr · 2017-02-08T22:12:10Z

src/components/titlePane/createTitlePane.ts

+	mixin: {
+		baseClasses: css,
+
+		onClickTitle: function (this: TitlePane) {


Should this be named onTitleClick instead? Just preference, but it would be good to stay consistent with other components. Also, all of these functions can be written without the function keyword:

... onTitleClick(this: TitlePane) { // ... } ...

I gave this some thought as I was writing it and I think "on<Action><Qualifier>" makes the most sense. I feel like I also found existing usage of this form, but I can't recall where.

Whatever you prefer, just try to make sure the existing components are updated as well to stay consistent.

bitpshr · 2017-02-08T22:18:36Z

src/components/titlePane/createTitlePane.ts

+				properties: {
+					collapsed = false
+				}
+			} = this;


The following may be cleaner, negating the need for this.properties four other times in this function:

const { collapsed = false, onRequestExpand, onRequestCollapse } = this.properties;

I'm not a fan of local variables without good justification:

local default: collapsed gets a local default of false, the others are used as-is

frequent repeated access: name lookup time is reduced with a local variable

excessive verbosity: more subjective, but for me this.properties.someThing does not cross the threshold of being too verbose

I'd argue that having to use this.properties.someThing four additional times, well as a lengthier destructuring, is excessively verbose...

bitpshr · 2017-02-08T22:21:21Z

src/components/titlePane/createTitlePane.ts

+			}
+		},
+
+		getChildrenNodes: function (this: TitlePane): DNode[] {


If you rebase against master, widgets will be using the most recent release of widget-core. Now, getChildrenNodes is replaced by a render function. This function should return a single top-level DNode, the children of which should be what you're currently returning here. See the Dialog or SlidePanel for reference.

bitpshr · 2017-02-08T22:27:41Z

src/components/titlePane/createTitlePane.ts

+				v('div', titleProperties, [ title ])
+			];
+
+			if (!collapsed) {


This should probably be if (collapsible && !collapsed) so a non-collapsible TitlePane can't be collapsed programmatically.

Thanks, good catch!

bitpshr · 2017-02-08T22:29:08Z

src/components/titlePane/createTitlePane.ts

+	}
+});
+
+export default createTitlePane;


I don't care what suffix we use ("Panel" vs. "Pane" vs. "BestDamnLayoutWidget"), but we should stay consistent with other components.

bitpshr · 2017-02-08T22:31:24Z

src/components/titlePane/styles/titlePane.css

+	background-color: var(--title-bar-bg);
+	border-color: var(--title-bar-bg);
+	border-style: solid;
+	border-width: 1px;


This should be a CSS variable, e.g. var(--title-pane-border-width). Don't worry about getting specific.

bitpshr · 2017-02-08T22:31:43Z

src/components/titlePane/styles/titlePane.css

+	border-width: 1px;
+	color: var(--title-bar-color);
+	font-size: var(--title-font-size);
+	padding: 5px;


This should be a variable.

bitpshr · 2017-02-08T22:32:00Z

src/components/titlePane/styles/titlePane.css

+.content {
+	border-color: var(--title-bar-bg);
+	border-style: solid;
+	border-width: 1px;


This should be the same variable used above for the .title border width.

msssk · 2017-02-08T23:40:52Z

Something that still needs to be figured out is how to animate collapse/expand.

translateY looks very nice, but does not animate the position of the element below - that element just snaps up and down.

max-height is what I am using, but it doesn't work well if you don't use a good value. Right now it's 300px, which of course will be bad for content taller than that. But if I set it to say, 2000px, then for 100px tall content the first 95% of the animation has no effect and all the visible animation happens in 5% of the time.

tomdye · 2017-02-09T09:28:59Z

src/components/titlePanel/createTitlePanel.ts

+			baseClasses: css,
+
+			onClickTitle(this: TitlePanel) {
+				const {


I'd be tempted to drop the nested destructure in favour of const { collapsed } = this.properties.
Could also destructure out onRequestExpand and onRequestCollapse. You could even consider defaulting them both to a noop such that they can both just be called without the checks.

Allocating extra function objects just to avoid null checks doesn't strike me as a good approach.

Mangala, you don't have to use noop defaults if you don't want to (I didn't in SlidePane), but I'd still destructure fully to avoid the four extra uses of this.properties.

I prefer the clarity of knowing precisely where the information is coming from at the time it's being used, and don't find this case to be excessively verbose, but I can change it.

this.properties.onRequestClose - at a glance I know exactly where onRequestClose came from
onRequestClose - at a glance I don't know where this came from, I have to look at other code

tomdye · 2017-02-09T09:29:58Z

src/components/titlePanel/createTitlePanel.ts

+		mixin: {
+			baseClasses: css,
+
+			onClickTitle(this: TitlePanel) {


I think the naming convention for this should be onTitleClick, but I could be wrong.

We should figure out and document a convention - I think on<Action><Qualifier> is preferable myself.

Consensus in the Dojo 2 room was for on<Qualifier><Action>, so onTitleClick.

tomdye · 2017-02-09T09:38:40Z

src/components/titlePanel/createTitlePanel.ts

+import * as animations from '../../styles/animations.css';
+
+export interface TitlePanelProperties extends WidgetProperties {
+	collapsed?: boolean;


I'd recommend swapping collapsed for open as we've used open elsewhere and it seems that you're needing to negate collapsed for most use cases.

+1, and maybe closeable instead of collapsible.

tomdye · 2017-02-09T09:39:38Z

src/components/titlePanel/styles/titlePanel.css

+@import '../../../styles/variables.css';
+
+:root {
+	--title-panel-border-width: 1px;


I think these are likely standard component variables / styling and thus should go to variables.css (with different names)

tomdye · 2017-02-09T09:40:00Z

src/components/titlePanel/createTitlePanel.ts

+import * as animations from '../../styles/animations.css';
+
+export interface TitlePanelProperties extends WidgetProperties {
+	collapsed?: boolean;


I'd recommend swapping collapsed for open as we've used open elsewhere and it seems that you're needing to negate collapsed for most use cases.

tomdye · 2017-02-09T09:43:51Z

src/styles/animations.css

+	}
+}
+
+.slideInDown {


I think we need to change this name, in animate-js this is the class / naming for sliding something in and down from off the page. I think this should be slideDown or expandDown We likely need to be cleverer than this though as we want to make sure the text doesn't move about as we expand and it may be better to fade that in at the same time? Worth playing with a little. Also, where did 300px come from for a max height? Seems restrictive, what are other frameworks / solutions doing?

Seems like they're all doing $(node).collapsible, but I don't think we want to go that way? 😉

I know this solution is not good and would like some more discussion on what we want to do. Are we OK with programmatic animations? Not sure there's a good way to do this in CSS alone.

These were originally sliding animations, which personally I think is a nice effect, but collapse/expand seems more common for accordions, and in any case, what we currently have is collapse/expand. Renamed to expandDown and collapseUp.

tomdye · 2017-02-09T09:44:09Z

src/components/titlePanel/createTitlePanel.ts

+import * as animations from '../../styles/animations.css';
+
+export interface TitlePanelProperties extends WidgetProperties {
+	collapsed?: boolean;


I'd recommend swapping collapsed for open as we've used open elsewhere and it seems that you're needing to negate collapsed for most use cases.

tomdye · 2017-02-09T09:47:01Z

src/components/titlePanel/createTitlePanel.ts

+					v('div', titleProperties, [ title ])
+				];
+
+				if (collapsible && !collapsed) {


I don't think you need to have collapsible here, would be better if !collapsed was replaced with open and defaulted to true.

collapsible is necessary so a non-collapsible TitlePane can't be closed programmatically.

The change from collapsible to closeable destroyed this logic, which is fine with me - as long as we enforce UI consistency, I feel OK with allowing programmatic actions that are disabled in the UI.

The logic should still work:

if (!closeable || (closeable && open)) { ... }

tomdye · 2017-02-09T14:55:01Z

src/components/titlePanel/createTitlePanel.ts

+					}, this.children));
+				}
+
+				return v('div', {


No need for the children array you created, you can do this:

return v('div', { classes: this.classes(css.main).get() }, [ v('div', titleProperties, [ title ]), !collapsed ? v('div', { classes: this.classes(css.content).get(), enterAnimation, exitAnimation }, this.children): null ]);

I don't like ternary usage except for very simple and short (in character count) cases. Perhaps it's a matter of preference between a functional/declarative approach versus procedural. I'd rather read more lines of code that are each simple, grokkable at a glance, and sequentially and clearly build up to a solution than fewer lines of code that have to be stared at for quite some time to decipher what all is being accomplished.

That being so, the example given above is consistent with how widgets and examples of v are shown. The null is filtered out by v.

So how about we improve widget and v examples?

rishson · 2017-02-09T16:46:36Z

src/components/titlePanel/createTitlePanel.ts

@@ -0,0 +1,90 @@
+import { VNodeProperties } from '@dojo/interfaces/vdom';


Before we land this, can we also add a11y support please?

@smhigley will have good suggestions for what's needed. You can omit focus / tabindex stuff for now until we get a focus manager.

I'll add specific comments, but I'm working off the a11y specs in https://www.w3.org/TR/wai-aria-practices-1.1/#accordion

bitpshr · 2017-02-09T21:40:11Z

@msssk, looks like we decided to go with Pane, not Panel. Just a heads up. master will be updated shortly.

smhigley · 2017-02-10T00:39:00Z

src/components/titlePanel/createTitlePanel.ts

+					titleProperties = {
+						classes: this.classes(css.title, css.collapsible).get(),
+						onclick: this.onClickTitle
+					};


The title should have a role="heading" and ideally an aria-level="#". The heading level may need to be added to properties, if we use it. The title div should also wrap a button element with the following properties:

aria-controls="id of content div"

aria-expanded: boolean set to collapsed

aria-disabled: boolean set to !collapsible

id attribute for the content div to reference

smhigley · 2017-02-10T00:41:01Z

src/components/titlePanel/createTitlePanel.ts

+						classes: this.classes(css.content).get(),
+						enterAnimation,
+						exitAnimation
+					}, this.children));


The content div needs an id for the title to reference, as well as aria-labelledby="id of title button"

smhigley · 2017-02-10T06:37:27Z

src/components/titlePanel/createTitlePanel.ts

+
+				if (open) {
+					// FIXME - id of content node
+					titleButtonProperties['aria-controls'] = 'content';


The button should actually have aria-controls="id string" set all the time, both open and closed. (Incidentally, I believe Matt G recommended uuid to generate unique id's, if it helps)

This seems very awkward to me since with vdom, in the closed state there is no content pane existing in the DOM. So we set aria-controls="<id of non-existent element>"?

smhigley · 2017-02-10T06:40:33Z

src/components/titlePanel/createTitlePanel.ts

 				const children: DNode[] = [
-					v('div', titleProperties, [ title ])
+					v('div', titleProperties, [
+						v('div', titleButtonProperties, [ title ])


Nitpicking, sorry! I personally think a button element is preferable to a div, but if you prefer the div, it'll also need tabindex="0" set (a focus manager wouldn't set initial tabindex values). You'd also need to make sure keyboard events (enter and space) trigger the content opening -- I'm not completely sure they work by default on a fake button.

I left it as a div since the w3 example doesn't use a button, and I wanted to avoid having to override button styles. But now I realize their example uses <a>, which gets much of the behavior that button has. I'd rather override button styles than reimplement button behavior, so I'll use <button> here.

bitpshr

Looking good Mangala.

bitpshr · 2017-02-10T13:56:56Z

src/components/titlePanel/createTitlePanel.ts

+				let titleButtonProperties: any = {
+					'aria-disabled': String(!closeable),
+					'aria-expanded': String(open),
+					// FIXME - set unique id


You can use @dojo/core/uuid for this, see the Dialog for an example.

bitpshr · 2017-02-10T13:57:09Z

src/components/titlePanel/createTitlePanel.ts

+			baseClasses: css,
+
+			onClickTitle(this: TitlePanel) {
+				const {


bitpshr · 2017-02-10T13:58:11Z

src/components/titlePanel/createTitlePanel.ts

+					])
+				];
+
+				if (open) {


Thinking about it more, a non-closeable Dialog can be closed programmatically, so this is fine.

bitpshr · 2017-02-10T14:03:09Z

src/components/titlePanel/createTitlePanel.ts

+		mixin: {
+			baseClasses: css,
+
+			onClickTitle(this: TitlePanel) {


Consensus in the Dojo 2 room was for on<Qualifier><Action>, so onTitleClick.

tomdye · 2017-02-10T14:38:07Z

Can you please drop the camelCase naming on the folder name. ie. tabPanel -> tabpane.

matt-gadd · 2017-02-10T17:22:41Z

src/components/titlePanel/createTitlePanel.ts

+					};
+				}
+				else {
+					titleProperties = {


does this not break the second rule of maquette? (and elsewhere for these properties?) http://maquettejs.org/docs/rules.html

this is a good reason in itself not to procedurally build up properties btw 😄

matt-gadd · 2017-02-10T17:34:45Z

src/examples/titlePanel/index.html

+<body>
+	<script src="../../../../node_modules/@dojo/loader/loader.min.js"></script>
+	<!-- TODO: should come from project dependency -->
+	<script src="https://cdnjs.cloudflare.com/ajax/libs/maquette/2.4.2/css-transitions.min.js"></script>


I've seen this elsewhere in the codebase but i'll ask anyway, is there any reason this cannot be loaded from node_modules/maquette/dist/css-transitions.min.js?

agubler · 2017-02-13T22:23:56Z

src/components/titlepane/TitlePane.ts

+};
+
+@theme(css)
+export default class TitlePane extends ThemeableMixin(WidgetBase)<TitlePaneProperties> {


We need to set the result of ThemeableMixin(WidgetBase) to a const and use the result to extend from.

const TitlePaneBase = ThemeableMixin(WidgetBase); export default class TitlePane extends TitlePaneBase<TitlePaneProperties> {

agubler · 2017-02-13T22:28:41Z

src/components/titlepane/TitlePane.ts

+
+@theme(css)
+export default class TitlePane extends ThemeableMixin(WidgetBase)<TitlePaneProperties> {
+	onClickTitle(event: MouseEvent & TouchEvent) {


Do we need the event param?

msssk · 2017-02-24T03:14:59Z

Test coverage is up to 100%.

Outstanding issues to resolve:

What do we do with content when the TitlePane is collapsed? @smhigley?
- Current behavior: don't render the content, and don't set the aria-controls attribute on the title button
- If we are going to set aria-controls then we should render the content. How do we hide it in an a11y-friendly manner?
How do we handle animation?
- There are only a few CSS properties that animate smoothly, and the currently used max-height is not one of them. Also max-height is a poor solution because we don't know what height the content will be.
- An animation function would give us precise control and handle arbitrary heights, but we don't have support for that

smhigley · 2017-02-27T22:21:35Z

What do we do with content when the TitlePane is collapsed?

Either the content or an element containing the content needs to be always present in the DOM, and its ID should be set in aria-controls. It (or its children) can be hidden with display: none or visibility: hidden, or the content can be added/removed from the DOM, so long as the containing element is present.

How do we handle animation?

I agree, I've never liked the max-height trick, but all the other options involve calculating height. What I've done in the past is clone the content node and position it offscreen with the correct width and box styles, and use it to calculate height.

(@msssk ^)

msssk · 2017-02-28T17:36:00Z

Thanks @smhigley that makes sense - DOM structure is now consistent, it's only the content nodes that are omitted when collapsed.

Now the remaining issues are:

expand/collapse animations
shared CSS: right now there is a hidden class in TitlePane.css - do we have a structure for sharing common classes like this?

smhigley · 2017-02-28T17:35:57Z

src/titlepane/TitlePane.ts

+				onclick: closeable ? this.onClickTitle : undefined,
+				role: 'heading'
+			}, [
+				v('div', {


All the changes look good! One very, very last thing and then I'm satisfied a11y-wise. The button just needs to either be an element that takes focus by default (<button> or <a>), or have tabindex set to 0.

Thanks, done! (tabIndex is 0, but if closeable is false then tabIndex is -1)

smhigley · 2017-02-28T17:39:20Z

@msssk the changes look great! I think the hidden class would be a good candidate for a util css file along with visuallyHidden.

For animations, maybe max-height is as good as we'll get for now if you think it's better to not mess around with DOM height calculations. Here's an example where it's done well: https://codepen.io/chriswrightdesign/pen/cmanI

- Move titlePane.css to titlePane.m.css - Fix typo - Lowercase component attribute name(s)

- Destructure state variables in the example - Change "titlePanel" to "titlePane" in example `key`s - Store IDs as private variables - Fix typo in doc comments - Use `Keys` from `common/util`

msssk requested review from agubler, matt-gadd and tomdye February 8, 2017 21:09

dylans added this to the 2017.02 milestone Feb 8, 2017

bitpshr suggested changes Feb 8, 2017

View reviewed changes

tomdye requested changes Feb 9, 2017

View reviewed changes

rishson reviewed Feb 9, 2017

View reviewed changes

dylans changed the title ~~TitlePane~~ TitlePanel Feb 9, 2017

smhigley suggested changes Feb 10, 2017

View reviewed changes

dylans changed the title ~~TitlePanel~~ TitlePane Feb 10, 2017

bitpshr suggested changes Feb 10, 2017

View reviewed changes

matt-gadd reviewed Feb 10, 2017

View reviewed changes

agubler reviewed Feb 13, 2017

View reviewed changes

msssk force-pushed the title-pane branch from 072c77f to 3f43237 Compare February 23, 2017 18:40

dylans modified the milestones: 2017.03, 2017.02 Feb 28, 2017

smhigley suggested changes Feb 28, 2017

View reviewed changes

smhigley approved these changes Feb 28, 2017

View reviewed changes

msssk and others added 21 commits May 17, 2017 11:15

TitlePane: add 'tabIndex' to toggle button

b8b2b89

updates

8710546

Use top margin approach to animations

ab33a3e

Update tests

bf55c9a

animations: remove unused CSS

7cfed03

TitlePane: update example

680da75

TitlePane: add keyboard support

0bf7527

TitlePane: fix typo

7d38e54

TitlePane: add web component wrapper

b7d0b7f

TitlePane: add keyboard tests

ccc6e5a

TitlePane: provide TitlePane instance to onRequestClose/Open

3bdc504

updates

b4e5ae0

Updates after rebase

97862ec

Test fixes

4711887

TitlePane updates

2d215cd

- Move titlePane.css to titlePane.m.css - Fix typo - Lowercase component attribute name(s)

Update TitlePane to use Dojo theme.

e3601cf

Update setProperties to __setProperties__

db914ad

PR feedback updates

ae9b758

- Destructure state variables in the example - Change "titlePanel" to "titlePane" in example `key`s - Store IDs as private variables - Fix typo in doc comments - Use `Keys` from `common/util`

Convert TitlePane tests to use harness

172fa75

Add basic TitlePane functional tests.

6952d6a

Cast diff functions as any

46b873e

msssk force-pushed the title-pane branch from 2f73ae4 to 46b873e Compare May 17, 2017 16:44

eheasley added beta2 and removed beta2 labels May 30, 2017

eheasley modified the milestones: 2017.05, 2017.06 Jun 6, 2017

eheasley assigned bitpshr Jun 6, 2017

bitpshr closed this Jun 8, 2017

		@@ -0,0 +1,90 @@
		import { VNodeProperties } from '@dojo/interfaces/vdom';

TitlePane #26

TitlePane #26

Conversation

msssk commented Feb 8, 2017 • edited Loading

codecov-io commented Feb 8, 2017 • edited by codecov bot Loading

Codecov Report

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msssk commented Feb 8, 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

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

bitpshr commented Feb 9, 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

bitpshr 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

tomdye commented Feb 10, 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

msssk commented Feb 24, 2017

smhigley commented Feb 27, 2017 • edited Loading

msssk commented Feb 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smhigley commented Feb 28, 2017 • edited Loading

msssk commented Feb 8, 2017 •

edited

Loading

codecov-io commented Feb 8, 2017 •

edited by codecov bot

Loading

tomdye commented Feb 10, 2017 •

edited

Loading

smhigley commented Feb 27, 2017 •

edited

Loading

smhigley commented Feb 28, 2017 •

edited

Loading