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.

Sign up for GitHub

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

Add helpertext to select #696

Merged
merged 2 commits into from
Apr 9, 2019
Merged

Add helpertext to select #696

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9ab46f2
add helpertext to select
tomdye Mar 22, 2019
f7a447e
remove extra helperText css import
tomdye Apr 9, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 27 additions & 0 deletions src/select/example/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,33 @@ export default class App extends WidgetBase {
this._value3 = option;
this.invalidate();
}
}),
v('br'),
w(Select, {
key: 'select4',
...this.getOptionSettings(),
getOptionSelected: (option: any) => !!this._value1 && option.value === this._value1,
label: 'Native select with helper text',
options: this._selectOptions,
useNativeElement: true,
value: this._value1,
onChange: (option: any) => {
this._value1 = option.value;
this.invalidate();
},
helperText: 'pick a value'
}),
w(Select, {
key: 'select5',
...this.getOptionSettings(),
label: 'Custom select box with helper text',
options: this._moreSelectOptions,
value: this._value2,
onChange: (option: any) => {
this._value2 = option.value;
this.invalidate();
},
helperText: 'pick a value'
})
]);
}
Expand Down
58 changes: 25 additions & 33 deletions src/select/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,12 +9,14 @@ import { v, w } from '@dojo/framework/widget-core/d';
import { uuid } from '@dojo/framework/core/util';
import { find } from '@dojo/framework/shim/array';
import { formatAriaProperties, Keys } from '../common/util';
import { CustomAriaProperties, LabeledProperties, InputProperties } from '../common/interfaces';
import { CustomAriaProperties, InputProperties } from '../common/interfaces';
import Icon from '../icon/index';
import Label from '../label/index';
import Listbox from '../listbox/index';
import HelperText from '../helper-text/index';
import * as css from '../theme/select.m.css';
import { customElement } from '@dojo/framework/widget-core/decorators/customElement';
import { helperText } from '../theme/text-input.m.css';

/**
* @type SelectProperties
Expand All @@ -32,20 +34,23 @@ import { customElement } from '@dojo/framework/widget-core/decorators/customElem
* @property useNativeElement Use the native <select> element if true
* @property value The current value
*/
export interface SelectProperties extends ThemedProperties, InputProperties, FocusProperties, LabeledProperties, CustomAriaProperties {
export interface SelectProperties extends ThemedProperties, InputProperties, FocusProperties, CustomAriaProperties {
getOptionDisabled?(option: any, index: number): boolean;
getOptionId?(option: any, index: number): string;
getOptionLabel?(option: any): DNode;
getOptionText?(option: any): string;
getOptionSelected?(option: any, index: number): boolean;
getOptionValue?(option: any, index: number): string;
helperText?: string;
options?: any[];
placeholder?: string;
useNativeElement?: boolean;
onBlur?(key?: string | number): void;
onChange?(option: any, key?: string | number): void;
onFocus?(key?: string | number): void;
value?: string;
labelHidden?: boolean;
label?: string;
}

@theme(css)
Expand All @@ -69,10 +74,9 @@ export interface SelectProperties extends ThemedProperties, InputProperties, Foc
'required',
'invalid',
'disabled',
'labelAfter',
'labelHidden'
],
attributes: [ 'widgetId', 'placeholder', 'label', 'value' ],
attributes: [ 'widgetId', 'placeholder', 'label', 'value', 'helperText' ],
events: [
'onBlur',
'onChange',
Expand Down Expand Up @@ -210,26 +214,6 @@ export class Select extends ThemedMixin(FocusMixin(WidgetBase))<SelectProperties
this._closeSelect();
}

protected getRootClasses() {
const {
disabled,
invalid,
readOnly,
required
} = this.properties;
const focus = this.meta(Focus).get('root');

return [
css.root,
disabled ? css.disabled : null,
focus.containsFocus ? css.focused : null,
invalid === true ? css.invalid : null,
invalid === false ? css.valid : null,
readOnly ? css.readonly : null,
required ? css.required : null
];
}

protected renderExpandIcon(): DNode {
const { theme, classes } = this.properties;
return v('span', { classes: this.theme(css.arrow) }, [
Expand Down Expand Up @@ -410,8 +394,8 @@ export class Select extends ThemedMixin(FocusMixin(WidgetBase))<SelectProperties
const {
label,
labelHidden,
labelAfter,
disabled,
helperText,
widgetId = this._baseId,
invalid,
readOnly,
Expand All @@ -420,9 +404,21 @@ export class Select extends ThemedMixin(FocusMixin(WidgetBase))<SelectProperties
theme,
classes
} = this.properties;

const focus = this.meta(Focus).get('root');

const children = [
return v('div', {
key: 'root',
classes: this.theme([
css.root,
disabled ? css.disabled : null,
focus.containsFocus ? css.focused : null,
invalid === true ? css.invalid : null,
invalid === false ? css.valid : null,
readOnly ? css.readonly : null,
required ? css.required : null
])
}, [
label ? w(Label, {
theme,
classes,
Expand All @@ -434,13 +430,9 @@ export class Select extends ThemedMixin(FocusMixin(WidgetBase))<SelectProperties
hidden: labelHidden,
forId: widgetId
}, [ label ]) : null,
useNativeElement ? this.renderNativeSelect() : this.renderCustomSelect()
];

return v('div', {
key: 'root',
classes: this.theme(this.getRootClasses())
}, labelAfter ? children.reverse() : children);
useNativeElement ? this.renderNativeSelect() : this.renderCustomSelect(),
w(HelperText, { theme, text: helperText })
Copy link
Contributor

Choose a reason for hiding this comment

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

Would you want to conditionally render the helper text widget? Something like helperText ? w(HelperText, { theme, text: helperText }) : null

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 wanted to render it at all times. The helpertext has both a root and a conditional text node. The reasoning behind this was to allow the consumer to theme it to reserve it's space if they wish in order to stop the UI jumping around if they helpertext shows / hides. In reality, it's best practise to always show a helperText for a widget that will be validated.

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it, alright 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess the render possess is cheap enough where you don't need to worry about that then

]);
}
}

Expand Down
24 changes: 21 additions & 3 deletions src/select/tests/unit/Select.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import {
compareAriaControls,
stubEvent
} from '../../../common/tests/support/test-helpers';
import HelperText from '../../../helper-text/index';

const harness = createHarness([ compareId, compareWidgetId, compareAriaControls ]);

Expand Down Expand Up @@ -55,6 +56,7 @@ interface ExpectedOptions {
classes?: any[];
overrides?: any;
focus?: boolean;
helperText?: string;
}

const testOptions: any[] = [
Expand Down Expand Up @@ -208,7 +210,7 @@ const expectedSingle = function(useTestProperties = false, withStates = false, o
return vdom;
};

const expected = function(selectVdom: any, { classes = [ css.root, null, null, null, null, null, null ], label = false, states, focus = false }: ExpectedOptions = {}) {
const expected = function(selectVdom: any, { classes = [ css.root, null, null, null, null, null, null ], label = false, states, focus = false, helperText }: ExpectedOptions = {}) {
return v('div', {
key: 'root',
classes
Expand All @@ -223,7 +225,8 @@ const expected = function(selectVdom: any, { classes = [ css.root, null, null, n
required: undefined,
forId: ''
}, [ 'foo' ]) : null,
selectVdom
selectVdom,
w(HelperText, { theme: undefined, text: helperText })
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you need to have theme as a property here?

Copy link
Member

Choose a reason for hiding this comment

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

Don't we want to pass the actual theme through here?

Copy link
Member Author

Choose a reason for hiding this comment

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

thats a test @agubler

Copy link
Member

Choose a reason for hiding this comment

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

Urgh my bad, damn not using a top level test directory... that’s what I checked for 😂

]);
};

Expand All @@ -240,6 +243,15 @@ registerSuite('Select', {
h.expect(() => expected(expectedNative()));
},

'helperText'() {
const h = harness(() => w(Select, {
options: testOptions,
useNativeElement: true,
helperText: 'foo'
}));
h.expect(() => expected(expectedNative(), { helperText: 'foo' }));
},

'custom properties'() {
const h = harness(() => w(Select, {
...testStateProperties,
Expand Down Expand Up @@ -325,6 +337,11 @@ registerSuite('Select', {
h.expect(() => expected(expectedSingle()));
},

'helperText'() {
const h = harness(() => w(Select, { helperText: 'foo' }));
h.expect(() => expected(expectedSingle(), { helperText: 'foo' }));
},

'custom properties'() {
const h = harness(() => w(Select, testStateProperties));
h.expect(() => expected(expectedSingle(true, true), {
Expand Down Expand Up @@ -469,7 +486,8 @@ registerSuite('Select', {
onOptionSelect: noop
})
])
])
]),
w(HelperText, { theme: undefined, text: undefined})
]));
},

Expand Down