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

Toolbar #397

Merged
merged 12 commits into from
Dec 13, 2017
3 changes: 2 additions & 1 deletion src/common/example/example.css
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
@import '../styles/animations.m.css';
@import '../styles/base.m.css';
@import '../styles/icons.m.css';
@import '../../accordionpane/styles/accordionPane.m.css';
@import '../../button/styles/button.m.css';
@import '../../calendar/styles/calendar.m.css';
@import '../../checkbox/styles/checkbox.m.css';
Expand All @@ -18,5 +19,5 @@
@import '../../textinput/styles/textinput.m.css';
@import '../../timepicker/styles/timePicker.m.css';
@import '../../titlepane/styles/titlePane.m.css';
@import '../../accordionpane/styles/accordionPane.m.css';
@import '../../toolbar/styles/toolbar.m.css';
@import '../../tooltip/styles/tooltip.m.css';
3 changes: 2 additions & 1 deletion src/common/example/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ const modules = [
'textinput',
'timepicker',
'titlepane',
'toolbar',
'tooltip'
];

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

render() {
return v('div', [
return v('div', { id: 'module-select' }, [
Copy link
Member Author

Choose a reason for hiding this comment

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

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.

v('h2', {
innerHTML: 'Select a module to view'
}),
Expand Down
1 change: 1 addition & 0 deletions src/common/tests/functional/all.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,4 +15,5 @@ import '../../../textarea/tests/functional/Textarea';
import '../../../textinput/tests/functional/TextInput';
import '../../../timepicker/tests/functional/TimePicker';
import '../../../titlepane/tests/functional/TitlePane';
import '../../../toolbar/tests/functional/Toolbar';
import '../../../tooltip/tests/functional/Tooltip';
1 change: 1 addition & 0 deletions src/common/tests/unit/all.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,4 +31,5 @@ import '../../../textarea/tests/unit/Textarea';
import '../../../textinput/tests/unit/TextInput';
import '../../../timepicker/tests/unit/TimePicker';
import '../../../titlepane/tests/unit/TitlePane';
import '../../../toolbar/tests/unit/Toolbar';
import '../../../tooltip/tests/unit/Tooltip';
2 changes: 1 addition & 1 deletion src/themes/dojo/slidePane.m.css
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@
font-size: 0;
padding: var(--grid-base);
position: absolute;
right: calc(2 * var(--grid-base));
right: var(--grid-base);
top: 50%;
transform: translateY(-50%);
}
Expand Down
2 changes: 2 additions & 0 deletions src/themes/dojo/theme.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import * as tabController from './tabController.m.css';
import * as textarea from './textarea.m.css';
import * as textinput from './textinput.m.css';
import * as titlePane from './titlePane.m.css';
import * as toolbar from './toolbar.m.css';
import * as tooltip from './tooltip.m.css';

export default {
Expand All @@ -39,5 +40,6 @@ export default {
'dojo-textarea': textarea,
'dojo-textinput': textinput,
'dojo-titlePane': titlePane,
'dojo-toolbar': toolbar,
'dojo-tooltip': tooltip
};
40 changes: 40 additions & 0 deletions src/themes/dojo/toolbar.m.css
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
@import './variables.css';

.toolbar {
background: var(--color-background);
border: var(--border-width) solid var(--color-border);
box-sizing: border-box;
height: calc(7 * var(--grid-base));
padding: calc(2 * var(--grid-base));
text-align: left;
}

.title {
font-size: var(--font-size-title);
margin: 0;
font-weight: normal;
}

.action {
padding-right: calc(3 * var(--grid-base));
}

.action:last-of-type {
padding-right: 0;
}

.menuButton {
background: none;
border: none;
cursor: pointer;
font-size: 0;
padding: var(--grid-base);
position: absolute;
right: var(--grid-base);
top: 50%;
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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?

transform: translateY(-50%);
}

.menuButton i {
font-size: var(--font-size-title);
}
4 changes: 4 additions & 0 deletions src/themes/dojo/toolbar.m.css.d.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
export const action: string;
export const root: string;
export const sticky: string;
export const title: string;
1 change: 1 addition & 0 deletions src/themes/dojo/widgets.css
Original file line number Diff line number Diff line change
Expand Up @@ -17,4 +17,5 @@
@import './textarea.m.css';
@import './textinput.m.css';
@import './titlePane.m.css';
@import './toolbar.m.css';
@import './tooltip.m.css';
43 changes: 43 additions & 0 deletions src/toolbar/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
# @dojo/widgets/toolbar/Toolbar widget

Dojo 2's `Toolbar` component can be used to display an application navigation bar that can show a title and action items. Action elements can be configured to automatically collapse into a `SlidePane` component, and the `Toolbar` itself can be configured to either be fixed or statically positioned.

## Features

- Capable of auto-collapsing action elements into `SlidePane`
- Capable of rendering either a fixed or statically positioned root element
- Customizable `title` element

### Accessibility Features

The menu button displays screen-readable text that's visually hidden and its icon has an `aria-hidden` attribute. However, the `Toolbar` component 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".)

## Themeing

The following CSS classes are used to style the `Toolbar` widget and should be provided by custom themes:

- `action`: Applied to the parent of each action item
- `actions`: Applied to the parent of the actions container
- `collapsed`: Applied to the root node if the root width is less than `collapseWidth`
- `menuButton`: Applied to the menu trigger
- `root`: Applied to the top-level wrapper node
- `sticky`: Applied to the root node if `fixed` is `true`
- `title`: Applied to the node containing the title element

## Example Usage

*Basic Example*
```typescript
import Toolbar from '@dojo/widgets/toolbar/Toolbar';
import { w } from '@dojo/widget-core/d';

w(Titlepane, {
title: 'My Site',
fixed: true,
collapseWidth: 720
}, [
v('a', { href: '/#home' }, [ 'Home' ]),
v('a', { href: '/#about' }, [ 'About' ]),
v('a', { href: '/#contact' }, [ 'Contact' ])
])
```
171 changes: 171 additions & 0 deletions src/toolbar/Toolbar.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,171 @@
import { Dimensions } from '@dojo/widget-core/meta/Dimensions';
import { DNode, WidgetProperties } from '@dojo/widget-core/interfaces';
import { ThemedMixin, theme } from '@dojo/widget-core/mixins/Themed';
import { v, w } from '@dojo/widget-core/d';
import { WidgetBase } from '@dojo/widget-core/WidgetBase';

import SlidePane, { Align } from '../slidepane/SlidePane';
import * as css from './styles/toolbar.m.css';
import * as iconCss from '../common/styles/icons.m.css';

/**
* Enum for toolbar positioning
*/
export const enum Position {
bottom = 'onBottomFixed',
top = 'onTopFixed'
};

/**
* @type ToolbarProperties
*
* 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
Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

I'm happy with that here

Copy link
Member

Choose a reason for hiding this comment

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

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

* @property fixed Fixes the toolbar to the top of the viewport
* @property onCollapse Called when action items change their layout
* @property position Determines toolbar position in relation to child contet
* @property title Element to show as the toolbar title
*/
export interface ToolbarProperties extends WidgetProperties {
actions?: DNode[];
collapseWidth?: number;
fixed?: boolean;
onCollapse?(collapsed: boolean): void;
position?: Position;
title?: DNode;
};

export const ThemedBase = ThemedMixin(WidgetBase);

@theme(css)
@theme(iconCss)
export default class Toolbar extends ThemedBase<ToolbarProperties> {
private _collapsed = false;
private _open = false;

constructor() {
super();
window.addEventListener('resize', this._collapseIfNecessary);
}

private _closeMenu() {
this._open = false;
this.invalidate();
}

private _collapseIfNecessary = () => {
const { collapseWidth = 800, onCollapse } = this.properties;
const { width } = this.meta(Dimensions).get('root').size;

if (width > collapseWidth && this._collapsed === true) {
this._collapsed = false;
onCollapse && onCollapse(this._collapsed);
this.invalidate();
}
else if (width <= collapseWidth && this._collapsed === false) {
this._collapsed = true;
onCollapse && onCollapse(this._collapsed);
this.invalidate();
}
}

private _toggleMenu() {
this._open = !this._open;
this.invalidate();
}

protected getFixedRootClasses(): (string | null)[] {
const { fixed, position = Position.top } = this.properties;
return [
css.rootFixed,
fixed ? css.stickyFixed : null,
(css as any)[position]
];
}

protected getRootClasses(): (string | null)[] {
return [
css.root,
this._collapsed ? css.collapsed : null,
this.properties.fixed ? css.sticky : null
];
}

protected onAttach() {
this._collapseIfNecessary();
}

protected onDetach() {
window.removeEventListener('resize', this._collapseIfNecessary);
}

protected renderActions(): DNode {
const { actions = [], theme } = this.properties;

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

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.


if (actionsElements.length === 0) {
return null;
}

return this._collapsed ? w(SlidePane, {
align: Align.right,
closeText: 'close menu',
key: 'slide-pane-menu',
onRequestClose: this._closeMenu,
open: this._open,
theme,
title: 'Menu'
}, actionsElements) : v('div', {
classes: [ this.theme(css.actions), css.actionsFixed ],
key: 'menu'
}, actionsElements);
}

protected renderButton(): DNode {
return this._collapsed ? v('button', {
classes: [ this.theme(css.menuButton), css.menuButtonFixed ],
onclick: this._toggleMenu
}, [
'open menu',
Copy link
Contributor

Choose a reason for hiding this comment

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

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

v('i', {
classes: this.theme([ iconCss.icon, iconCss.barsIcon ]),
role: 'presentation', 'aria-hidden': 'true'
})
]) : null;
}

protected renderTitle(): DNode {
const { title } = this.properties;

return title ? v('div', {
Copy link
Member

Choose a reason for hiding this comment

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

no h1 ?

classes: [ this.theme(css.title), css.titleFixed ]
}, [ title ]) : null;
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

}

render(): DNode {
const classes = this.getRootClasses();
const fixedClasses = this.getFixedRootClasses();

return v('div', {
classes: [ ...this.theme(classes), ...fixedClasses ],
key: 'root'
}, [
v('div', {
classes: [ this.theme(css.toolbar), css.toolbarFixed ]
}, [
this.renderTitle(),
this.renderActions(),
this.renderButton()
]),
v('div', {
classes: [ this.theme(css.content), css.contentFixed ]
}, this.children)
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added!

]);
}
}
35 changes: 35 additions & 0 deletions src/toolbar/createToolbarElement.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
import { CustomElementDescriptor } from '@dojo/widget-core/customElements';
import Toolbar from './Toolbar';

/**
* Configures a Toolbar web component
*/
export default function createToolbarElement(): CustomElementDescriptor {
return {
tagName: 'dojo-toolbar',
widgetConstructor: Toolbar,
attributes: [
{
attributeName: 'collapseWidth',
value: value => Number(value)
},
{
attributeName: 'fixed',
value: value => value === 'false' || value === '0' ? false : true
},
{
attributeName: 'title'
}
],
properties: [
{ propertyName: 'actions' },
{ propertyName: 'theme' }
],
events: [
{
propertyName: 'onCollapse',
eventName: 'collapse'
}
]
};
}
Loading