-
Notifications
You must be signed in to change notification settings - Fork 65
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
Toolbar #397
Changes from 8 commits
70be07f
a896b19
0ec4896
693edf5
6ce0a4d
ef68fa0
211edf7
ae568ce
e4a762f
d085d7d
64da254
66cfdca
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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%; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can you not use There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
} |
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; |
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' ]) | ||
]) | ||
``` |
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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm happy with that here There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Might want an |
||
* @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 ])); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.:
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', | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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', { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. no |
||
classes: [ this.theme(css.title), css.titleFixed ] | ||
}, [ title ]) : null; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wonder if this should really be an There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added! |
||
]); | ||
} | ||
} |
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' | ||
} | ||
] | ||
}; | ||
} |
There was a problem hiding this comment.
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 anypadding-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.