-
Notifications
You must be signed in to change notification settings - Fork 2.9k
New Component: ComboBox #1920
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
New Component: ComboBox #1920
Changes from 3 commits
6d18606
3cc47d2
ec78224
3399566
e43ac7b
ac35bdc
d4c84ec
6b2ef29
aad70fb
1c6b74b
1612bcc
3941b20
d52e4e3
4b779c6
ba63a49
8a0a24c
8db368f
b1a1f6e
2be2043
f61ce25
51e4e21
f8e0956
b312e36
97c208f
80f901e
2670a5d
d02daaa
398bdd1
12b7b7b
3536017
55e2ad7
c6b98ed
460f3de
5bd6c64
3022036
358d025
705a852
2634c1b
6b0e678
9039ab4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,33 @@ | ||
| import * as React from 'react'; | ||
| import { ComboBoxPage } from 'office-ui-fabric-react/lib/components/ComboBox/ComboBoxPage'; | ||
| import { PageHeader } from '../../components/PageHeader/PageHeader'; | ||
| import { ComponentPage } from '../../components/ComponentPage/ComponentPage'; | ||
|
|
||
| export class ComboBoxComponentPage extends React.Component<any, any> { | ||
| public render() { | ||
| return ( | ||
| <div ref='pageElement'> | ||
| <ComponentPage> | ||
| <PageHeader pageTitle='ComboBox' backgroundColor='#038387' | ||
| links={ | ||
| [ | ||
| { | ||
| 'text': 'Overview', | ||
| 'location': 'Overview' | ||
| }, | ||
| { | ||
| 'text': 'Variants', | ||
| 'location': 'Variants' | ||
| }, | ||
| { | ||
| 'text': 'Implementation', | ||
| 'location': 'Implementation' | ||
| } | ||
| ] | ||
| } /> | ||
| <ComboBoxPage isHeaderVisible={ false } /> | ||
| </ComponentPage> | ||
| </div> | ||
| ); | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| export * from './components/ComboBox/index'; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,56 @@ | ||
| import { IDropdownOption, IDropdownProps } from '../../Dropdown'; | ||
| import { IIconProps } from '../../Icon'; | ||
|
|
||
| export interface IComboBox { | ||
|
|
||
| } | ||
|
|
||
| export interface IComboBoxProps extends IDropdownProps { | ||
|
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 we use composition instead of inheritance? 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. Ideally a combo box is just a dropdown + input text so that should be reflected in the props as well, IMO 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 more conflicted now that the DropDown component is not even used.
Contributor
Author
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. The comboBox doesn't use a dropdown since dropdowns allow you to style the visible collapsed where comboBoxes enforce that the rendering is inside of a |
||
|
|
||
| /** | ||
| * Collection of options for this ComboBox | ||
| */ | ||
| options?: IComboBoxOption[]; | ||
|
|
||
| /** | ||
| * Callback issues when either: | ||
| * 1) the selected option changes | ||
| * 2) a manually edited value is submitted. In this case there may not be a matched option if allowFreeform is also true | ||
| * (and hence only value would be true, the other parameter would be null in this case) | ||
| */ | ||
| onChanged?: (option?: IComboBoxOption, index?: number, value?: string) => void; | ||
|
|
||
| /** | ||
| * Callback issued when the options should be resolved, if they have been updated or | ||
| * if they need to be passed in the first time | ||
| */ | ||
| onResolveOptions?: () => IComboBoxOption[] | PromiseLike<IComboBoxOption[]>; | ||
|
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's the difference between this function returning IComboBoxOption[] and setting options directly?
Contributor
Author
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. One is handling the full state outside of the component (e.g. setting the options directly) opposed to the uncontrolled case where it may take some time to get the options back but we want the state to be stored inside of the component. The promise allows for the update without blocking |
||
|
|
||
| /** | ||
| * Whether the ComboBox is free form, meaning that the user input is not bound to provided items. Defaults to false. | ||
| */ | ||
| allowFreeform?: boolean; | ||
|
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 this be renamed to something like unconstrainedInput?
Contributor
Author
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. We could, but free form text is a common term: http://www.pcmag.com/encyclopedia/term/43488/free-form-text OR http://www.dictionaryofengineering.com/definition/free-form-text.html |
||
|
|
||
| /** | ||
| * Whether the ComboBox auto completes. As the user is inputing text, it will be suggested potential matches from the list of items. If | ||
| * the combo box is expanded, this will also scroll to the suggested item, and give it a selected style. Defaults to false. | ||
| */ | ||
| autoComplete?: boolean; | ||
|
|
||
| /** | ||
| * Value to show in the input, does not have to map to a combobox option | ||
| */ | ||
| value?: string; | ||
|
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. should be mutually exclusive with defaultSelectedKey and selectedKey? 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's the scenario that requires this extra prop?
Contributor
Author
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. Sure, I'll add the mutually exclusive warning. Really, you could have a selectedKey and a default value to show if desired, but I can't think of a case where you would not want to use the selected options value if it was known
Contributor
Author
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. The scenario that requires this prop is the font size combobox, it allows freeform and will allow for input like "11.5" as valid, but will not add that value as an option in the menu. Another case where is could be needed is when all the options are gotten at a later time than when the component is rendered. This allows the creator pass in the value to show without having 1) have any other info, 2) duplicate data just to make the rendering work 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. thanks, that make sense. I wonder if renaming the variable can make that clearer. |
||
|
|
||
| /** | ||
| * The IconProps to use for the button aspect of the combobox | ||
| */ | ||
| buttonIconProps?: IIconProps; | ||
| } | ||
|
|
||
| export interface IComboBoxOption extends IDropdownOption { | ||
|
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. This inheritance is fragile IMO, and some of the fields don't make sense to be inherited, DropdownMenuItemType for example. Why not extract an ISelectableOption interface that both Dropdown and Combobox can use and have the both components decouple?
Contributor
Author
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. Sure, I'll rework this. It does make sense to have the contents of the DropdownMenuItemType although a better name would make it easier to think about |
||
| /** | ||
| * Font-family associated with this option. | ||
| */ | ||
| fontFamily?: string; | ||
|
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. this is interesting, isn't this specific to our font type combo box? Isn't there already an onRenderItem or similar that would allow the consumer to specify how to render each item?
Contributor
Author
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. True, but I felt like this aspect is scoped enough to expose it as well without making the creator create an onRenderOption
Contributor
Author
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. Question for you @jair-cazarin : Do you want to handle this with onRenderOption for our use case? If so I can remove the IComboBoxOption 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. it feels very specific to that particular ComboBox IMO 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. so I'd vote to remove it |
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,266 @@ | ||
| @import '../../common/common'; | ||
|
|
||
| // Copyright (c) Microsoft. All rights reserved. Licensed under the MIT license. See LICENSE in the project root for license information. | ||
|
|
||
| // | ||
| // Office UI Fabric | ||
| // -------------------------------------------------- | ||
| // ComboBox styles | ||
|
|
||
| $ComboBox-selectedItem-bg: $ms-color-neutralQuaternaryAlt; | ||
| $ComboBox-selectedItem-hover-bg: $ms-color-neutralLighter; | ||
| $ComboBox-height: 32px; | ||
| $ComboBox-caretDown-width: 32px; | ||
| $ComboBox-item-height: 36px; | ||
|
|
||
| // Mixin for high contrast mode link states | ||
| @mixin highContrastListItemState { | ||
| @media screen and (-ms-high-contrast: active) { | ||
| background-color: $ms-color-contrastBlackSelected; | ||
| border-color: $ms-color-contrastBlackSelected; | ||
| color: $ms-color-black; | ||
|
|
||
| &:focus { | ||
| border-color: $ms-color-black; | ||
| } | ||
| } | ||
|
|
||
| @media screen and (-ms-high-contrast: black-on-white) { | ||
| background-color: $ms-color-contrastWhiteSelected; | ||
| border-color: $ms-color-contrastWhiteSelected; | ||
| color: $ms-color-white; | ||
| } | ||
|
|
||
| @include highContrastAdjust(); | ||
| } | ||
|
|
||
| .root { | ||
| @include ms-normalize; | ||
| @include ms-font-m; | ||
| color: $ms-color-neutralPrimary; | ||
|
|
||
| margin-bottom: 10px; | ||
| position: relative; | ||
| outline: 0; | ||
| user-select: none; | ||
| background: $ms-color-white; | ||
| border: 1px solid $ms-color-neutralTertiaryAlt; | ||
| cursor: text ; | ||
| display: block; | ||
| height: $ComboBox-height; | ||
| line-height: $ComboBox-height - 2px; // height minus the border | ||
| @include padding(0, $ComboBox-caretDown-width, 0, 0); | ||
| overflow: hidden; | ||
| white-space: nowrap; | ||
| text-overflow: ellipsis; | ||
|
|
||
|
|
||
|
|
||
| &:hover, | ||
| &.focused { | ||
| &.wrapper | ||
| { | ||
| border-color: $ms-color-themePrimary; | ||
|
|
||
| @media screen and (-ms-high-contrast: active) { | ||
| color: $ms-color-contrastBlackSelected; | ||
| } | ||
|
|
||
| @media screen and (-ms-high-contrast: black-on-white) { | ||
| color: $ms-color-contrastWhiteSelected; | ||
| } | ||
| } | ||
|
|
||
| &.readOnly { | ||
| .input, | ||
| .caretDown { | ||
| background-color: $ms-color-neutralLighter; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| &.wrapperForError { | ||
| border-color: $ms-color-error; | ||
| } | ||
|
|
||
| :global(.ms-Label) { | ||
| display: inline-block; | ||
| margin-bottom: 8px; | ||
| } | ||
| } | ||
|
|
||
| //== State: A disabled ComboBox | ||
| .root.rootIsDisabled { | ||
| &.wrapper, | ||
| .input, | ||
| .caretDown { | ||
| background-color: $ms-color-neutralLighter; | ||
| border-color: $ms-color-neutralLighter; | ||
| color: $ms-color-neutralTertiary; | ||
| cursor: default; | ||
|
|
||
| @media screen and (-ms-high-contrast: active) { | ||
| border-color: $ms-color-contrastBlackDisabled; | ||
| color: $ms-color-contrastBlackDisabled; | ||
| } | ||
|
|
||
| @media screen and (-ms-high-contrast: black-on-white) { | ||
| border-color: $ms-color-contrastWhiteDisabled; | ||
| color: $ms-color-contrastWhiteDisabled; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| .input { | ||
| box-sizing: border-box; | ||
| width: 100%; | ||
| height: 100%; | ||
| border: none; | ||
| outline: none; | ||
| font: inherit; | ||
| text-overflow: ellipsis; | ||
| padding-left: 12px; | ||
|
|
||
| &:hover { | ||
| background-color: $ms-color-neutralLight; | ||
| color: $ms-color-neutralPrimary; | ||
| } | ||
|
|
||
| &::selection { | ||
| background-color: $ms-color-neutralSecondaryAlt; | ||
| color: $ms-color-white; | ||
| } | ||
| } | ||
|
|
||
| .caretDown { | ||
| color: $ms-color-neutralDark; | ||
| font-size: $ms-icon-size-s; | ||
| position: absolute; | ||
| height: $ComboBox-height; | ||
| line-height: $ComboBox-height - 2px; // height minus the border | ||
| width: $ComboBox-caretDown-width; | ||
| text-align: center; | ||
| cursor: default; | ||
|
|
||
| &:hover{ | ||
| background-color: $ms-color-neutralQuaternaryAlt; | ||
| } | ||
|
|
||
| &:active{ | ||
| background-color: $ms-color-neutralTertiaryAlt; | ||
| } | ||
| } | ||
|
|
||
| .callout { | ||
| box-shadow: 0 0px 5px 0px rgba(0, 0, 0, 0.4); | ||
| border: 1px solid $ms-color-neutralLight; | ||
| max-height: 576px; | ||
| } | ||
|
|
||
| .errorMessage{ | ||
| color: $ms-color-error; | ||
| &::before { | ||
| content: '* '; | ||
| } | ||
| } | ||
|
|
||
| .items { | ||
| display: block; | ||
| } | ||
|
|
||
| // Container for the ComboBox items, displayed as a panel on small screens. | ||
| .item { | ||
| background: transparent; | ||
| box-sizing: border-box; | ||
| cursor: pointer; | ||
| display: block; | ||
| width: 100%; | ||
| height: auto; | ||
| min-height: $ComboBox-item-height; | ||
| line-height: 20px; | ||
| padding: 5px 16px; | ||
| position: relative; | ||
| border: 1px solid transparent; | ||
| word-wrap: break-word; | ||
| overflow-wrap: break-word; | ||
| text-align: left; | ||
|
|
||
| @media screen and (-ms-high-contrast: active) { | ||
| border-color: $ms-color-black; | ||
| } | ||
|
|
||
| @media screen and (-ms-high-contrast: black-on-white) { | ||
| border-color: $ms-color-white; | ||
| } | ||
|
|
||
| &:hover { | ||
| background-color: $ComboBox-selectedItem-hover-bg; | ||
| color: $ms-color-black; | ||
|
|
||
| @include highContrastListItemState; | ||
| } | ||
|
|
||
| @include focus-border(); | ||
|
|
||
| &:focus { | ||
| background-color: $ms-color-neutralLighter; | ||
| } | ||
|
|
||
| &:active { | ||
| background-color: $ComboBox-selectedItem-hover-bg; | ||
| color: $ms-color-black; | ||
| } | ||
|
|
||
| &.itemIsDisabled { | ||
| background: $ms-color-white; | ||
| color: $ms-color-neutralTertiary; | ||
| cursor: default; | ||
| } | ||
| :global(.ms-Button-flexContainer) { | ||
| justify-content: flex-start; | ||
| } | ||
| } | ||
|
|
||
| // A selected ComboBox item | ||
| .item.itemIsSelected { | ||
| background-color: $ComboBox-selectedItem-bg; | ||
| color: $ms-color-black; | ||
|
|
||
| &:hover { | ||
| background-color: $ComboBox-selectedItem-bg; | ||
| } | ||
|
|
||
| @include focus-border(); | ||
|
|
||
| @include highContrastListItemState; | ||
| } | ||
|
|
||
| .header { | ||
| @include ms-font-m; | ||
| font-weight: $ms-font-weight-semibold; | ||
| color: $ms-color-themePrimary; | ||
| background: none; | ||
| border: none; | ||
| height: $ComboBox-item-height; | ||
| line-height: $ComboBox-item-height; | ||
| cursor: default; | ||
| padding: 0px 16px; | ||
| user-select: none; | ||
| @include text-align(left); | ||
| } | ||
|
|
||
| .divider { | ||
| height: 1px; | ||
| background-color: $dividerColor; | ||
| } | ||
|
|
||
| .optionText { | ||
| overflow: hidden; | ||
| white-space: nowrap; | ||
| text-overflow: ellipsis; | ||
| min-width: 0px; | ||
| max-width: 100%; | ||
| word-wrap: break-word; | ||
| overflow-wrap: break-word; | ||
| margin: 1px; | ||
| } |
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.
why do you need an empty interface?
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.
yeah, it's not needed was just following the convention in the fabric codebase