Overflow set #1537
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,10 @@ | ||
| { | ||
| "changes": [ | ||
| { | ||
| "packageName": "office-ui-fabric-react", | ||
| "comment": "OverflowSet: New Overflow Set componet to create sets of elements with overflow showing in callout", | ||
| "type": "patch" | ||
| } | ||
| ], | ||
| "email": "micahgodbolt@gmail.com" | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| export * from './components/OverflowSet/index'; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,7 +1,7 @@ | ||
| import * as React from 'react'; | ||
| import { BaseButton } from '../BaseButton'; | ||
| import { BaseComponent, nullRender, assign } from '../../../Utilities'; | ||
| import { IButtonProps, IButtonClassNames } from '../Button.Props'; | ||
| import * as stylesImport from './CommandButton.scss'; | ||
| const styles: any = stylesImport; | ||
|
|
||
|
|
@@ -12,6 +12,7 @@ const CLASS_NAMES: IButtonClassNames = { | |
| menuIcon: styles.icon, | ||
| isDisabled: styles.isDisabled, | ||
| isEnabled: styles.isEnabled, | ||
| isOpened: styles.isOpened, | ||
| label: styles.label, | ||
| root: styles.root, | ||
| flexContainer: styles.flexContainer | ||
|
|
@@ -27,9 +28,10 @@ export class CommandButton extends BaseComponent<IButtonProps, {}> { | |
| public render() { | ||
| return ( | ||
| <BaseButton | ||
| { ...this.props } | ||
| classNames={ assign({}, CLASS_NAMES, this.props.classNames) } | ||
|
Member
There was a problem hiding this comment. Can we use the spread operator here instead of assign?
Member
Author
There was a problem hiding this comment. The assign is to allow prop classnames to override the button specific class names. i.e. I want a new .isEnabled:hover style, I can override their isEnabled with my own. And since it's css modules, even if I use the same isEnabled class name, the hash will be different.
Member
There was a problem hiding this comment. That should still work with object spread though. They are equivalent and is more compact: https://www.typescriptlang.org/docs/handbook/release-notes/typescript-2-1.html It would be something like this classNames = { ...CLASS_NAMES, ..this.props.classNames }
Member
Author
There was a problem hiding this comment. Pulled this out. @dzearing is doing this using glamor, and yes, spreads. |
||
| onRenderDescription={ nullRender } | ||
| /> | ||
| ); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -32,7 +32,7 @@ | |
|
|
||
| .isEnabled { | ||
|
|
||
| &:hover, &.isOpened { | ||
| background-color: $ms-color-themeDark; | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,28 @@ | ||
| import * as React from 'react'; | ||
| import { OverflowSet } from './OverflowSet'; | ||
| import { IContextualMenuItem } from '../../ContextualMenu'; | ||
| import { IButtonProps } from '../../Button'; | ||
| import { IRenderFunction } from '../../Utilities'; | ||
|
|
||
| export interface IOverflowSetProps extends React.Props<OverflowSet> { | ||
|
|
||
| /** | ||
| * An array of items to be rendered by your onRenderItem function in the primary content area | ||
| */ | ||
| items?: any[]; | ||
|
|
||
| /** | ||
| * An array of items to be passed to overflow contextual menu | ||
| */ | ||
| overflowItems?: IContextualMenuItem[]; | ||
|
Member
There was a problem hiding this comment. Why are the types different items vs overflow items?
Member
Author
There was a problem hiding this comment. OverflowItems are meant to be rendered in a contextual menu. Of course there isn't anything stopping someone from rendering them in something else....If any[] is prefered I have no problems either way.
Member
There was a problem hiding this comment. I think contextualmenuitem already has a mechanism to render anything else inside them, so I think it is ok. |
||
|
|
||
| /** | ||
| * Method to call when trying to render an item. | ||
| */ | ||
| onRenderItem: IRenderFunction<any>; | ||
|
|
||
| /** | ||
| * Rendering method for overflow button and contextual menu. | ||
| */ | ||
| onRenderOverflowButton: IRenderFunction<IButtonProps>; | ||
|
Member
There was a problem hiding this comment. I'm a little unsure about this API. We definitely want to be able to provide flexibility for rendering the overflow button, but it seems like we would have an onRenderMenuItem function for handling items in the contextual menu
Member
Author
There was a problem hiding this comment. The rendering of the menu items is already handled by the contextual menu. and in fact IContextualMenuItem can accept an onRender function already.
Member
There was a problem hiding this comment. Ah I see. I guess I was expecting the API to list "top level controls" and "overflow controls" in a similar manner. But given the existing mechanism for overriding menu items in contextualmenuitem, this seems to make sense and also allows customization of the button itself. |
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| @import '../../common/common'; | ||
|
|
||
| .root { | ||
| position: relative; | ||
| display: flex; | ||
| flex-wrap: nowrap; | ||
| } | ||
|
|
||
| .item { | ||
| flex-shrink: 0; | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,45 @@ | ||
| import * as React from 'react'; | ||
| import { | ||
| css, | ||
| autobind, | ||
| BaseComponent | ||
| } from '../../Utilities'; | ||
| import { IButtonProps } from '../../Button'; | ||
| import { IOverflowSetProps } from './OverflowSet.Props'; | ||
| import { FocusZone, FocusZoneDirection } from '../../FocusZone'; | ||
|
|
||
| import * as stylesImport from './OverflowSet.scss'; | ||
| const styles: any = stylesImport; | ||
|
|
||
| export class OverflowSet extends BaseComponent<IOverflowSetProps, null> { | ||
|
|
||
| public render() { | ||
| let { | ||
| items, | ||
| overflowItems, | ||
| onRenderOverflowButton | ||
| } = this.props; | ||
|
|
||
| const overflowButtonProps: IButtonProps = { | ||
| menuProps: { items: overflowItems } | ||
| } | ||
|
|
||
| return ( | ||
| <FocusZone className={ css('ms-OverflowSet', styles.root) } direction={ FocusZoneDirection.horizontal } role='menubar' > | ||
| { items && this._onRenderItems(items) } | ||
| { overflowItems.length && onRenderOverflowButton(overflowButtonProps) } | ||
|
Member
Author
There was a problem hiding this comment. What do you mean by default value?
Member
There was a problem hiding this comment. For the onrenderoverflowbutton? I guess it makes sense given your other comment regarding how overflow items are rendered.
Member
Author
There was a problem hiding this comment. Yeah, i had a default button at one time, but what does the button look like? Hover/active/selected? I'm expecting each user will customize the button to their use case. A default button wouldn't serve much purpose unless we determined what the default usecase was.
Member
There was a problem hiding this comment. Seems reasonable, we can add it later if there is a "common" button that arises |
||
| </FocusZone> | ||
| ); | ||
| } | ||
|
|
||
| @autobind | ||
| private _onRenderItems(items: any[]): JSX.Element[] { | ||
| return items.map((item, i) => { | ||
| return ( | ||
| <div key={ i } className={ css('ms-OverflowSet-item', styles.item) }> | ||
|
Member
There was a problem hiding this comment. This isn't a great way to assign keys since it wouldn't let React's reconciler detect reorderings and insertions/deletions. More details can be found here https://facebook.github.io/react/docs/lists-and-keys.html Can we provide a way for the consumer to pass in a unique identifier for each item?
Member
Author
There was a problem hiding this comment. Yeah, i think I originally had that in the map. Let me put it back in. Look for a key and fall back to i if there is no key. |
||
| { this.props.onRenderItem(item) } | ||
| </div> | ||
| ); | ||
| }); | ||
| } | ||
| } | ||
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.
I just realized this is a really bad contract; for example, it is unclear from a user how to define the color on an icon that is disabled. We need to tweak this a bit.
Can we hold off adding something that would be deprecated fast just for a sec? Sorry, know this has been a long time.
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.
So no exposing of the IButtonClassNames or any of that? Revert all the changes on the button files?