Listbox #312

smhigley · 2017-10-17T07:52:20Z

Type: feature

The following has been addressed in the PR:

There is a related issue
All code matches the style guide
Unit or Functional tests are included in the PR

Description:

Refactors Select and ComboBox to use a common Listbox widget. Resolves #117 and #102. This PR also introduces some of the discussed changes to widget patterns, namely splitting the render() function into more granular, overridable functions.

pottedmeat · 2017-10-25T19:49:20Z

src/listbox/Listbox.ts

@@ -0,0 +1,264 @@
+import { WidgetBase } from '@dojo/widget-core/WidgetBase';


Imports should be alphabetical

pottedmeat · 2017-10-25T20:02:34Z

src/listbox/Listbox.ts

+	active?: boolean;
+	classes?: (string | null)[];
+	disabled?: boolean;
+	label: DNode;


Alphabetize

pottedmeat · 2017-10-25T20:17:51Z

src/listbox/Listbox.ts

+import * as css from './styles/listbox.m.css';
+
+/* Listbox Option sub-widget */
+export interface ListboxOptionProperties extends ThemeableProperties {


Can/should this be moved to its own file?

Good question 😂 . It used to be, then after talking with Ant/Matt G, the conclusion was that sub-widgets shouldn't really exist because they don't stand on their own, except that they make handling certain events much easier. I moved it out of its own file so it would seem less like a standalone widget, but I agree that it seemed better organized when separate

Moved back to its own file. We'll see what happens 😄

pottedmeat · 2017-10-25T20:18:12Z

src/listbox/Listbox.ts

+ * Properties that can be set on a Listbox component
+ *
+ * @property activeIndex          Index of the currently active listbox option
+ * @property customOption         Custom widget constructor for options. Should extend ListboxOption


customOption isn't in this interface

pottedmeat · 2017-10-25T20:19:57Z

src/listbox/Listbox.ts

+	describedBy?: string;
+	visualFocus?: boolean;
+	id?: string;
+	multiselect?: boolean;


multiselect not in TypeDoc

pottedmeat · 2017-10-25T20:51:15Z

src/listbox/Listbox.ts

+				active: activeIndex === index,
+				classes: this.getOptionClasses(activeIndex === index, disabled, selected),
+				disabled,
+				label: this.renderOptionLabel(option, index),


Consider moving this above (alongside the getOptionDisabled and getOptionSelected to standardize.

Should I break out id as well? I originally only pulled out disabled and selected because they're also used to get the state classes whereas label/id are only used once.

pottedmeat · 2017-10-25T20:53:27Z

src/listbox/Listbox.ts

+			activeIndex = 0,
+			key = '',
+			optionData = [],
+			getOptionDisabled = (option: any, index: number) => false,


Consider pulling this out into a reusable function at the top of the file

pottedmeat · 2017-10-25T20:56:42Z

src/combobox/ComboBox.ts

 import { ThemeableMixin, ThemeableProperties, theme } from '@dojo/widget-core/mixins/Themeable';
 import { WidgetBase } from '@dojo/widget-core/WidgetBase';
 import { diffProperty } from '@dojo/widget-core/decorators/diffProperty';
 import { reference } from '@dojo/widget-core/diff';
-import ResultItem from './ResultItem';
-import ResultMenu from './ResultMenu';
+import Listbox from '../listbox/Listbox';


Alphebetize

pottedmeat · 2017-10-25T21:08:53Z

src/combobox/ComboBox.ts

 	private _getResultLabel(result: any) {
 		const { getResultLabel } = this.properties;

-		return getResultLabel ? getResultLabel(result) : result;
+		return getResultLabel ? getResultLabel(result) : `${result}`;


Could this just be String(result)?

Template strings are the most optimal way to do string conversion now.

pottedmeat · 2017-10-25T21:20:26Z

src/select/Select.ts

- * @property disabled       Prevents the user from interacting with the form field
- * @property invalid        Indicates the value entered in the form field is invalid
- * @property label          Label settings for form label text, position, and visibility
- * @property multiple       Whether the widget supports multiple selection


How is this handled now?

Listbox on its own is effectively a custom multiselect widget, and the native multiselect is so universally awful that I don't think we need to go out of our way to support it.

tomdye · 2017-10-26T13:55:16Z

src/listbox/Listbox.ts

+
+	@diffProperty('activeIndex', auto)
+	protected calculateScroll(previousProperties: ListboxProperties, { activeIndex = 0 }: ListboxProperties) {
+		const scrollOffset = this.meta(Dimensions).get('root').scroll.top;


This should be a single call to this.meta(Dimensions) not multiple

bitpshr

Partial review. Things look good. I'm mostly confused by the apparent removal of certain pieces of functionality, like multiple on the Select, and the ability to pass custom options and menus to components. If this is supported, it's not immediately obvious based on the properties interfaces for each component that uses ListBox. I also had some various questions.

bitpshr · 2017-10-26T13:04:00Z

src/combobox/example/index.ts

@@ -61,68 +59,14 @@ const data = [
 	{ value: 'West Virginia' }
 ];

-class CustomResultItem extends ResultItem {


Why was the example of a custom result item removed?

bitpshr · 2017-10-26T13:04:14Z

src/combobox/example/index.ts

-	}
-}
-
-class CustomResultMenu extends ResultMenu {


Why was the example of a custom result menuremoved?

bitpshr · 2017-10-26T13:18:29Z

src/listbox/Listbox.ts

+	}
+
+	@diffProperty('activeIndex', auto)
+	protected calculateScroll(previousProperties: ListboxProperties, { activeIndex = 0 }: ListboxProperties) {


What is the purpose of this function? Is this what _scrollIntoView used to do?

More or less. I separated out calculateScroll and animateScroll so that the latter would be easy to override (e.g. to animate the scroll)

bitpshr · 2017-10-26T13:20:00Z

src/listbox/Listbox.ts

+	}
+
+	protected onElementUpdated(element: HTMLElement, key: string) {
+		if (key === 'root' && typeof this._scroll === 'number') {


Can you refactor this with the login in onElementCreated?

bitpshr · 2017-10-26T13:21:57Z

src/listbox/Listbox.ts

+			onOptionSelect
+		} = this.properties;
+
+		const disabled = getOptionDisabled(option, index);


It looks like you call this function a few different places, and also define it in each of those places. Could you pull this out into a static function somewhere? Or consider using a default property.

Never mind, default properties haven't landed.

bitpshr · 2017-10-26T13:42:23Z

src/listbox/example/index.ts

+import dojoTheme from '../../themes/dojo/theme';
+
+interface CustomOption {
+	value: string;


Nit, but alphabetize

bitpshr · 2017-10-26T13:43:50Z

src/listbox/example/index.ts

+				getOptionLabel: (option: CustomOption) => option.label,
+				getOptionDisabled: (option: CustomOption) => !!option.disabled,
+				getOptionSelected: (option: CustomOption) => !!option.selected,
+				onActiveIndexChange: (index: number) => {


I thought the types of the arguments for these callbacks could be inferred now, but I could be insane.

I get implicit any errors without the type ¯_(ツ)_/¯

bitpshr · 2017-10-26T13:44:37Z

src/select/Select.ts

@@ -18,382 +17,332 @@ import * as iconCss from '../common/styles/icons.m.css';
 *
 * Properties that can be set on a Select component
 *
- * @property CustomOption   Custom widget constructor for options. Should use SelectOption as a base


Is this no longer supported?

bitpshr · 2017-10-26T13:49:31Z

src/combobox/ComboBox.ts

 		this.invalidate();
 	}

-	protected onElementCreated(element: HTMLElement, key: string) {
+	protected onElementUpdated(element: HTMLElement, key: string) {


Can we remove reliance on onElement* methods? Technically they're sort of kind of deprecated, though the message was temporarily removed. I'm wondering if we can leverage meta in a way that negates the need for these altogether.

Would the animation meta be able to handle scroll? I suppose we could write our own within Listbox, but I'd rather use something that exists, if we can.

Moved to meta 👍

bitpshr · 2017-10-26T13:52:38Z

src/select/Select.ts

 }

 export const SelectBase = ThemeableMixin(WidgetBase);

 @theme(css)
 @theme(iconCss)
+@diffProperty('options', reference)


Was this working incorrectly before explicitly diffing by reference?

Yup, it doesn't call a render if you update the options property otherwise.

tomdye · 2017-10-26T14:00:32Z

src/listbox/Listbox.ts

+	}
+
+	protected animateScroll(element: HTMLElement, scrollValue: number) {
+		element.scrollTop = scrollValue;


I think the correct way to do this would be to create a local scrollTop meta within this file that takes a key and a scrollValue. That would avoid the need to use onElementCreated / onElementUpdated

codecov · 2017-10-31T09:48:27Z

Codecov Report

Merging #312 into master will decrease coverage by 0.2%.
The diff coverage is 98.15%.

@@            Coverage Diff            @@
##           master    #312      +/-   ##
=========================================
- Coverage   99.01%   98.8%   -0.21%     
=========================================
  Files          24      22       -2     
  Lines        1718    1753      +35     
  Branches      447     469      +22     
=========================================
+ Hits         1701    1732      +31     
+ Misses          1       0       -1     
- Partials       16      21       +5

Impacted Files	Coverage Δ
src/timepicker/TimePicker.ts	`100% <ø> (ø)`	⬆️
src/main.ts	`100% <100%> (ø)`	⬆️
src/select/Select.ts	`97.57% <97.14%> (-1.87%)`	⬇️
src/combobox/ComboBox.ts	`97.36% <97.95%> (-0.89%)`	⬇️
src/listbox/Listbox.ts	`99.16% <99.16%> (ø)`
src/slidepane/SlidePane.ts	`98.82% <0%> (+2.35%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a5765e1...3652b59. Read the comment docs.

codecov · 2017-10-31T09:48:27Z

Codecov Report

Merging #312 into master will decrease coverage by 0.19%.
The diff coverage is 98.5%.

@@            Coverage Diff            @@
##           master     #312     +/-   ##
=========================================
- Coverage   99.03%   98.84%   -0.2%     
=========================================
  Files          24       23      -1     
  Lines        1866     1901     +35     
  Branches      480      488      +8     
=========================================
+ Hits         1848     1879     +31     
- Partials       18       22      +4

Impacted Files	Coverage Δ
src/timepicker/TimePicker.ts	`100% <ø> (ø)`	⬆️
src/listbox/ListboxOption.ts	`100% <100%> (ø)`
src/select/Select.ts	`97.57% <97.14%> (-1.87%)`	⬇️
src/listbox/Listbox.ts	`99.03% <99.03%> (ø)`
src/combobox/ComboBox.ts	`98.4% <99.04%> (+0.14%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bae1322...96ec0af. Read the comment docs.

tomdye · 2017-11-01T10:37:19Z

src/listbox/Listbox.ts

+	id?: string;
+	multiselect?: boolean;
+	optionData?: any[];
+	ScrollMeta?: WidgetMetaConstructor<DefaultScroll>;


do you really think this might be passed in as a property? I think it's more likely that we end up moving it out into widget-core/meta entirely if it's used in multiple places.

I just want to make sure it's easy to customize, e.g. to animate the scroll or something. Do you think there's anything wrong with passing it in as a property? I suppose just having protected animateScroll could be enough, since someone could override it.

To me it feels somewhat overkill to provide both an extension point and an injection point. Should we not simply choose one pattern and stick to it?

Works for me. I'm fine removing the prop and leaving protected animateScroll as the point of extension 👍

tomdye · 2017-11-01T10:39:20Z

src/listbox/Listbox.ts

+		const menuDimensions = this.meta(Dimensions).get('root');
+		const scrollOffset = menuDimensions.scroll.top;
+		const menuHeight = menuDimensions.offset.height;
+		const optionOffset = this.meta(Dimensions).get(this._getOptionId(activeIndex)).offset;


does _getOptionId return an ID or a KEY?

It returns a unique string that's used in two places, once for an id and once for a key

bitpshr

@smhigley this is looking really good now. One question: was logic in onElement* methods actually moved to meta as commented? I still see the same focus functionality in those deprecated methods using direct DOM access. Other than that, I think we're at a point where merging this and fixing any nits after the fact is a good thing to do. Once the random CI failure about suite error is fixed :)

smhigley requested review from bitpshr and agubler October 17, 2017 14:59

pottedmeat mentioned this pull request Oct 18, 2017

Combobox: update unit tests to use test-extras #164

Closed

3 tasks

bitpshr mentioned this pull request Oct 20, 2017

Architecture of CSS Modules #278

Closed

dylans added this to the 2017.10 milestone Oct 20, 2017

smhigley force-pushed the listbox branch from 8a37a9d to cae2e86 Compare October 24, 2017 04:32

pottedmeat self-requested a review October 25, 2017 16:47

pottedmeat reviewed Oct 25, 2017

View reviewed changes

tomdye requested changes Oct 26, 2017

View reviewed changes

bitpshr suggested changes Oct 26, 2017

View reviewed changes

tomdye requested changes Oct 26, 2017

View reviewed changes

kitsonk removed this from the 2017.10 milestone Oct 30, 2017

tomdye reviewed Nov 1, 2017

View reviewed changes

smhigley force-pushed the listbox branch from 3652b59 to 7dd4579 Compare November 1, 2017 16:51

smhigley mentioned this pull request Nov 2, 2017

OptionData should live in Select instead of SelectOption #297

Closed

smhigley added 13 commits November 2, 2017 19:38

stuffs

a43f27f

listbox widget

ddaef54

initial combobox implementation

536b967

pass key to all events

b23ff7c

select integration, minus blur behavior

15186a6

listbox tests and better example page

4b9f309

listbox option tests

6b0574d

select unit tests

4e75b56

update select typedoc

328a2ee

combobox tests

3196de3

remove unused combobox properties from timepicker

d5c9d60

listbox scroll to option

4db206a

simplify listbox scroll, only use dimension meta

5908f14

smhigley added 9 commits November 2, 2017 19:38

listbox tests with scroll and updated option

4f56fc4

remove styles for testing scroll

f7e5191

updated unit tests for intern4

f8749d4

give select a min height

a4af0db

pr feedback

60bd9c3

use meta for scrolling

c2c948d

update option tests

00a835c

separated ListboxOption once again, added scroll meta test

2e486ac

fixed combobox tests

f396081

bitpshr approved these changes Nov 2, 2017

View reviewed changes

fixed test imports

5860e8b

smhigley force-pushed the listbox branch from 223f566 to 5860e8b Compare November 2, 2017 14:54

smhigley added 3 commits November 3, 2017 01:08

listbox functional tests, updated unit tests

8a5e51e

fixed functional tests

48fa879

keys are optional parameters, clean up branches

96ec0af

tomdye approved these changes Nov 6, 2017

View reviewed changes

smhigley merged commit b8a27c4 into dojo:master Nov 6, 2017

dylans added this to the beta.4 milestone Jan 10, 2018

		@@ -0,0 +1,264 @@
		import { WidgetBase } from '@dojo/widget-core/WidgetBase';

Listbox #312

Listbox #312

Conversation

smhigley commented Oct 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smhigley Oct 31, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bitpshr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Oct 31, 2017

Codecov Report

codecov bot commented Oct 31, 2017 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bitpshr left a comment • edited Loading

Choose a reason for hiding this comment

smhigley commented Oct 17, 2017 •

edited

Loading

smhigley Oct 31, 2017 •

edited

Loading

codecov bot commented Oct 31, 2017 •

edited

Loading

bitpshr left a comment •

edited

Loading