Toolbar #397

bitpshr · 2017-11-30T14:17:31Z

Type: 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 implements an opinionated Toolbar component that can configured to use either fixed or static positioning. Users can provide a title element and action elements. Action elements will optionally be rendered in a SlidePane when the component width is less than the collapseWidth. Both the title and actions can be any DNode, which means events can be attached directly to those elements instead of being propagated up through the Toolbar itself.

Resolves #386

bitpshr · 2017-11-30T14:20:53Z

src/common/example/index.ts

@@ -32,7 +33,7 @@ export class App extends WidgetBase<WidgetProperties> {
 	}

 	render() {
-		return v('div', [
+		return v('div', { id: 'module-select' }, [


The Toolbar uses a flexbox-based layout wherein page content should be a child of the toolbar itself. This negates the need for any padding-top on the <body> element to account for a fixed-positioned toolbar, and allows the toolbar to be an arbitrary height while still allowing page content to scroll independently.

Unfortunately, because our example pages render this module select UI before any component is rendered, it's impossible to place this inside the Toolbar example. To get around this, the toolbar example page adds a single CSS rule to the example page to style the select in a more sane manner, hence this added ID.

bitpshr · 2017-11-30T14:23:56Z

src/toolbar/Toolbar.ts

+ * Properties that can be set on a Toolbar component
+ *
+ * @property actions           Action elements to show in the toolbar
+ * @property collapseWidth     Width at which to collapse actions into a SlidePane


I feel strongly that we shouldn't always be inverting control to the parent for every piece of UI functionality. It's both tedious and common enough to hide action items in a SlidePane that our opinionated component should provide this, while also allowing easy extension. In practice, using a property like collapseWidth instead of collapsed and internalizing window resizing logic felt much cleaner.

I'm happy with that here

Might want an onCollapse(collapsed: boolean) callback though to notify parents?

bitpshr · 2017-11-30T14:25:12Z

src/toolbar/Toolbar.ts

+	constructor() {
+		super();
+		this._onResize = debounce(this._collapseIfNecessary, 250);
+		window.addEventListener('resize', this._onResize);


This is stopgap until dojo/widget-core#618 lands. For now, we do the best we can by debouncing a resize handler that's explicitly removed on detachment.

Does this need to be debounced given that it will already be limited by requestAnimationFrame

bitpshr · 2017-11-30T14:26:46Z

src/toolbar/example/index.ts

+	}
+
+	onAttach() {
+		const style = document.createElement('style');


This method is purely for demo styling purposes. See above comment re: Toolbar and its children.

bitpshr · 2017-11-30T14:27:12Z

src/toolbar/styles/toolbar.m.css

+	overflow-y: auto;
+}
+
+.action { }


We may not need all these classes to be themeable, but it felt right to expose as many of them as possible.

tomdye · 2017-11-30T14:46:08Z

src/themes/dojo/toolbar.m.css

+@import './variables.css';
+
+.toolbar {
+	background: #FFF;


can this be moved to a var?

tomdye · 2017-11-30T14:47:57Z

src/themes/dojo/toolbar.m.css

+	padding: var(--grid-base);
+	position: absolute;
+	right: var(--grid-base);
+	top: 50%;


can you not use align-items: center on the parent to achieve this?

Sort of, but it would require one extra element. Since I'm already absolutely positioning the button to the right it was a bit cleaner (and more uniform) to just use top and transform.

what about when our users want to use flexbox? If adding an extra element to make this simpler and removes absolute positioning etc is it not worth it?

tomdye · 2017-11-30T14:52:21Z

src/toolbar/Toolbar.ts

+ * Properties that can be set on a Toolbar component
+ *
+ * @property actions           Action elements to show in the toolbar
+ * @property collapseWidth     Width at which to collapse actions into a SlidePane


I'm happy with that here

tomdye · 2017-11-30T14:53:03Z

src/toolbar/Toolbar.ts

+ * Properties that can be set on a Toolbar component
+ *
+ * @property actions           Action elements to show in the toolbar
+ * @property collapseWidth     Width at which to collapse actions into a SlidePane


Might want an onCollapse(collapsed: boolean) callback though to notify parents?

tomdye · 2017-11-30T14:54:04Z

src/toolbar/Toolbar.ts

+	constructor() {
+		super();
+		this._onResize = debounce(this._collapseIfNecessary, 250);
+		window.addEventListener('resize', this._onResize);


Does this need to be debounced given that it will already be limited by requestAnimationFrame

tomdye · 2017-11-30T15:02:41Z

src/toolbar/Toolbar.ts

+		return this._collapsed ? w(SlidePane, {
+			align: Align.right,
+			closeText: 'close menu',
+			key: 'menu',


not sure you should have two elements with the same key

tomdye · 2017-11-30T15:03:27Z

src/toolbar/Toolbar.ts

+	protected renderTitle(): DNode {
+		const { title } = this.properties;
+
+		return title ? v('div', {


tomdye · 2017-11-30T15:04:09Z

src/toolbar/Toolbar.ts

+		];
+	}
+
+	protected getModifierClasses(): (string | null)[] {


renamed these to getRootClasses elsewhere

tomdye

Just the question re. flex remains

smhigley

I like it! Looks like a great addition to widgets :)

smhigley · 2017-12-06T17:05:05Z

src/toolbar/styles/toolbar.m.css

+	height: 100%;
+	left: 0;
+	overflow: hidden;
+	position: fixed;


Does this need to be fixed? It also works with position: absolute, and then it isn't constrained to only being document-level (although a toolbar on anything else seems weird, granted).

Good catch. The fixed styling was left over from the approach that didn't rely on content being a child of the Toolbar.

smhigley · 2017-12-06T17:08:22Z

src/toolbar/Toolbar.ts

+		const actionsElements = actions.map((action, index) => v('div', {
+			classes: [ this.theme(css.action) ],
+			key: index
+		}, [ action ]));


I don't really understand why these need to be wrapped in a div. Is it just so their css/theming is within the toolbar CSS as opposed to in parent-widget-level CSS?

Yes, this is really just for easier theming. Most situations I thought of were pretty difficult to accomplish without actions being grouped in a common parent. I'm open to removing it if necessary though.

Hm, I kind of imagine most of the time the actions will be passed in already wrapped in a parent, for semantics and styling. E.g.:

w(Toolbar, { actions: [ v('div', { id: 'menu', role: 'menubar' }, [ v('a', { href: '/#home' }, [ 'Home' ]), v('a', { href: '/#about' }, [ 'About' ]), v('a', { href: '/#contact' }, [ 'Contact' ]) ]) ], title: 'Foobar' }, [ content ])

In that case the extra wrapping div seems a little unnecessary. I don't really dislike it, I just think it introduces an extra wrapping element where none is needed.

smhigley · 2017-12-06T17:10:15Z

src/toolbar/Toolbar.ts

+
+		return title ? v('h1', {
+			classes: [ this.theme(css.title), css.titleFixed ]
+		}, [ title ]) : null;


I wonder if this should really be an h1. Usually that should go to a content-specific title, not to a company name/logo, which is more common for toolbars. Since title is already a DNode, could we just return that, wrapped in a div for flex styling?

p.s.: really like the breakdown of the render fn :)

smhigley · 2017-12-06T17:16:47Z

src/toolbar/tests/functional/Toolbar.ts

+				.then(displayed => {
+					assert.isFalse(displayed);
+				});
+	}


I know functional tests are a pain, but maybe one to make sure the slidepane is shown or not shown based on window width would be good

^ ignore that if it's too much time, or if you already tried and removed it in frustration

I think this is a good idea, but I had the hardest time getting functional tests to run locally. I can open a new issue for this if that works for you.

smhigley · 2017-12-06T17:17:16Z

src/tooltip/Tooltip.ts

@@ -12,7 +12,7 @@ import * as css from './styles/tooltip.m.css';
 *
 * @property content           Information to show within the tooltip
 * @property orientation       Where this tooltip should render relative to its child
- * @property open           Determines if this tooltip is visible
+ * @property open              Determines if this tooltip is visible


smhigley · 2017-12-06T17:21:38Z

src/toolbar/Toolbar.ts

+			]),
+			v('div', {
+				classes: [ this.theme(css.content), css.contentFixed ]
+			}, this.children)


Not sure if this needs to be added now, but reversing the order of the toolbar and content based on a position property would be easy, and having the option of a bottom toolbar is nice. Works with the styling as-is, too.

smhigley · 2017-12-06T17:27:25Z

src/toolbar/Toolbar.ts

+			classes: [ this.theme(css.menuButton), css.menuButtonFixed ],
+			onclick: this._toggleMenu
+		}, [
+			'open menu',


You probably already know this, but depending on the order of merging of this and Neil's i18n PR, the 'open menu'/'close menu' text should either be changed here, or Neil should know that you added some more strings for his PR

codecov · 2017-12-08T05:08:23Z

Codecov Report

Merging #397 into master will increase coverage by 0.72%.
The diff coverage is 89.65%.

@@            Coverage Diff             @@
##           master     #397      +/-   ##
==========================================
+ Coverage   97.14%   97.86%   +0.72%     
==========================================
  Files          26       27       +1     
  Lines        1997     2062      +65     
  Branches      517      531      +14     
==========================================
+ Hits         1940     2018      +78     
+ Misses         20        6      -14     
- Partials       37       38       +1

Impacted Files	Coverage Δ
src/tooltip/Tooltip.ts	`100% <ø> (ø)`	⬆️
src/toolbar/Toolbar.ts	`89.65% <89.65%> (ø)`
src/enhancedtextinput/EnhancedTextInput.ts	`100% <0%> (ø)`
src/textinput/TextInput.ts	`98.59% <0%> (+0.2%)`	⬆️
src/tabcontroller/TabController.ts	`99.04% <0%> (+0.95%)`	⬆️

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 645cb2d...66cfdca. Read the comment docs.

bitpshr · 2017-12-08T05:09:28Z

@smhigley This is ready for another review when you have a chance.

smhigley · 2017-12-11T17:24:50Z

src/toolbar/README.md

+
+### Accessibility Features
+
+- The menu button displays screen-readable text that's visually hidden and its icon has an `aria-hidden` attribute


Thinking about the menu wrapping, it might also be good to add some text like this:

Toolbar makes no assumptions about what content will be inserted through actions. If you wish to create a site-wide navigation section, it would be best to wrap links in an element with the appropriate semantics (e.g. <nav> or role="navigation")

smhigley

MERGE ALL THE THINGS

bitpshr requested review from pottedmeat, tomdye and smhigley November 30, 2017 14:17

bitpshr commented Nov 30, 2017

View reviewed changes

tomdye requested changes Nov 30, 2017

View reviewed changes

tomdye approved these changes Dec 5, 2017

View reviewed changes

smhigley reviewed Dec 6, 2017

View reviewed changes

bitpshr added 7 commits December 7, 2017 23:39

Initial toolbar implementation

70be07f

Update tests and README

a896b19

Use flexbox

0ec4896

PR feedback

693edf5

Add onCollapse property callback

6ce0a4d

Add theme to custom element descriptor

ef68fa0

PR feedback

211edf7

smhigley reviewed Dec 11, 2017

View reviewed changes

Update README and change h1 to div

ae568ce

smhigley approved these changes Dec 11, 2017

View reviewed changes

bitpshr added 4 commits December 12, 2017 15:26

Update Toolbar tests to no longer rely on window width

e4a762f

Skip functional tests on Safari due to unsupported WebDriver methods

d085d7d

Increase animation delay for functional tests

64da254

Use better displayed element check for functional tests/

66cfdca

bitpshr merged commit 4da7a15 into dojo:master Dec 13, 2017

bitpshr deleted the toolbar branch December 13, 2017 18:04

dylans added this to the beta.5 milestone Dec 15, 2017


		### Accessibility Features

		- The menu button displays screen-readable text that's visually hidden and its icon has an `aria-hidden` attribute

Toolbar #397

Toolbar #397

Conversation

bitpshr commented Nov 30, 2017 • edited by kitsonk 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomdye left a comment

Choose a reason for hiding this comment

smhigley 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

smhigley Dec 6, 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

codecov bot commented Dec 8, 2017 • edited Loading

Codecov Report

bitpshr commented Dec 8, 2017

Choose a reason for hiding this comment

smhigley left a comment

Choose a reason for hiding this comment

bitpshr commented Nov 30, 2017 •

edited by kitsonk

Loading

smhigley Dec 6, 2017 •

edited

Loading

codecov bot commented Dec 8, 2017 •

edited

Loading