Simplify widget render #336

smhigley · 2017-10-27T17:14:33Z

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:

Resolves #316

Makes widgets more easily extensible by simplifying the render code, particularly addressing these points from the issue:

break out large chunks of rendered dom
state classes are returned in a function
icons are broken out
inline styles are calculated separately

bitpshr

A few subjective observations / suggestions, but this is ready to land. I made #343 to track the removal of all other sub-widgets.

bitpshr · 2017-10-31T17:33:12Z

src/button/Button.ts

@@ -70,6 +70,30 @@ export default class Button extends ButtonBase<ButtonProperties> {
 	private _onTouchEnd (event: TouchEvent) { this.properties.onTouchEnd && this.properties.onTouchEnd(event); }
 	private _onTouchCancel (event: TouchEvent) { this.properties.onTouchCancel && this.properties.onTouchCancel(event); }

+	protected getContent() {


It may be nice to embrace ES getters if no arguments are passed to these:

get content() { return this.children; }

i'd say this is overkill @bitpshr especially if the getter ends up doing more than simply getting.

bitpshr · 2017-10-31T17:37:24Z

src/calendar/tests/unit/Calendar.ts

 	return w(CalendarCell, {
-		key: `date-${dateIndex}`,
+		key: `date-${date}${active ? '' : '-dimmed'}`,


Why was this change necessary?

when I moved rendering the actual date cell into its own function (renderDateCell), I didn't have access to the index any more. It seemed overkill to pass it in just for the key when I could also generate a unique key based on the date and currentMonth.

bitpshr · 2017-10-31T17:37:55Z

src/calendar/Calendar.ts

@@ -16,7 +16,6 @@ import * as iconCss from '../common/styles/icons.m.css';
 *
 * Properties that can be set on a Calendar component
 *
- * @property CustomDateCell    Custom widget constructor for the date cell. Should use CalendarCell as a base.


Did you remove sub-widgets as part of this PR as well?

The problem is sub-widgets make it much easier to handle data binding on event handlers (e.g. onClick on a specific date cell returns the correct date and disabled state). Based on talking with Ant, I think the compromise reached was to leave in sub-widgets, but try not to expose them to the user

I've been wondering if it makes sense to remove the typedoc for CalendarCell. It's marginally useful to developers wanting to extend CalendarCell, but I definitely don't want it showing up in Dojo documentation

tomdye · 2017-10-31T17:46:12Z

src/button/Button.ts

@@ -70,6 +70,30 @@ export default class Button extends ButtonBase<ButtonProperties> {
 	private _onTouchEnd (event: TouchEvent) { this.properties.onTouchEnd && this.properties.onTouchEnd(event); }
 	private _onTouchCancel (event: TouchEvent) { this.properties.onTouchCancel && this.properties.onTouchCancel(event); }

+	protected getContent() {


i'd say this is overkill @bitpshr especially if the getter ends up doing more than simply getting.

tomdye · 2017-10-31T17:49:04Z

src/calendar/Calendar.ts

+		});
+	}
+
+	protected renderPagingButtonContent(type: 'next' | 'previous') {


next / previous would be better as a string enum

tomdye · 2017-10-31T17:49:51Z

src/calendar/DatePicker.ts

@@ -191,7 +191,38 @@ export default class DatePicker extends DatePickerBase<DatePickerProperties> {
 		onPopupChange && onPopupChange(this._getPopupState());
 	}

-	private _renderMonthRadios() {
+	protected renderControlsTrigger(type: 'month' | 'year'): DNode {


string enum

tomdye · 2017-10-31T17:50:35Z

src/calendar/DatePicker.ts

@@ -215,7 +246,20 @@ export default class DatePicker extends DatePickerBase<DatePickerProperties> {
 		]));
 	}

-	private _renderYearRadios() {
+	protected renderPagingButtonContent(type: 'next' | 'previous') {


string enum

codecov · 2017-11-01T17:42:01Z

Codecov Report

Merging #336 into master will increase coverage by 0.22%.
The diff coverage is 99.68%.

@@            Coverage Diff             @@
##           master     #336      +/-   ##
==========================================
+ Coverage    98.9%   99.13%   +0.22%     
==========================================
  Files          24       24              
  Lines        1740     1845     +105     
  Branches      462      467       +5     
==========================================
+ Hits         1721     1829     +108     
+ Misses          1        0       -1     
+ Partials       18       16       -2

Impacted Files	Coverage Δ
src/splitpane/SplitPane.ts	`98.57% <100%> (+0.13%)`	⬆️
src/tabcontroller/TabController.ts	`98.09% <100%> (+0.01%)`	⬆️
src/dialog/Dialog.ts	`97.67% <100%> (-0.06%)`	⬇️
src/calendar/DatePicker.ts	`99.27% <100%> (+0.06%)`	⬆️
src/calendar/Calendar.ts	`99.46% <100%> (+0.03%)`	⬆️
src/radio/Radio.ts	`98.27% <100%> (+0.23%)`	⬆️
src/button/Button.ts	`100% <100%> (ø)`	⬆️
src/slider/Slider.ts	`98.78% <100%> (+0.29%)`	⬆️
src/calendar/CalendarCell.ts	`100% <100%> (ø)`	⬆️
src/tabcontroller/TabButton.ts	`100% <100%> (ø)`	⬆️
... and 11 more

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 d33c943...aba806b. Read the comment docs.

codecov · 2017-11-01T17:42:01Z

Codecov Report

Merging #336 into master will increase coverage by 0.12%.
The diff coverage is 99.68%.

@@            Coverage Diff             @@
##           master     #336      +/-   ##
==========================================
+ Coverage    98.9%   99.03%   +0.12%     
==========================================
  Files          24       24              
  Lines        1740     1866     +126     
  Branches      462      480      +18     
==========================================
+ Hits         1721     1848     +127     
+ Misses          1        0       -1     
  Partials       18       18

Impacted Files	Coverage Δ
src/calendar/Calendar.ts	`99.46% <100%> (+0.03%)`	⬆️
src/calendar/CalendarCell.ts	`100% <100%> (ø)`	⬆️
src/button/Button.ts	`100% <100%> (ø)`	⬆️
src/textinput/TextInput.ts	`100% <100%> (ø)`	⬆️
src/slider/Slider.ts	`98.78% <100%> (+0.29%)`	⬆️
src/tabcontroller/TabController.ts	`98.09% <100%> (+0.01%)`	⬆️
src/dialog/Dialog.ts	`98.07% <100%> (+0.34%)`	⬆️
src/checkbox/Checkbox.ts	`98.59% <100%> (+0.15%)`	⬆️
src/timepicker/TimePicker.ts	`100% <100%> (ø)`	⬆️
src/titlepane/TitlePane.ts	`98.33% <100%> (+0.18%)`	⬆️
... and 6 more

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 d33c943...570dcc7. Read the comment docs.

pottedmeat

Other than minor stuff, I think these new methods should have return types.

pottedmeat · 2017-10-27T17:35:30Z

src/button/Button.ts

@@ -70,6 +70,30 @@ export default class Button extends ButtonBase<ButtonProperties> {
 	private _onTouchEnd (event: TouchEvent) { this.properties.onTouchEnd && this.properties.onTouchEnd(event); }
 	private _onTouchCancel (event: TouchEvent) { this.properties.onTouchCancel && this.properties.onTouchCancel(event); }

+	protected getContent() {


Can we specify return types for these utility functions?

pottedmeat · 2017-10-31T17:59:13Z

src/calendar/Calendar.ts

-		const { renderWeekdayCell } = this.properties;
-		return renderWeekdayCell ? renderWeekdayCell(day) : v('abbr', { title: day.long }, [ day.short ]);
+	protected renderDateCell(date: number, selected: boolean, currentMonth: boolean, today: boolean): DNode {
+		const key = currentMonth ? `date-${date}` : `date-${date}-dimmed`;


Should more/all the calculated values be extracted to the calling method?

This matches up with the comment on the unit test where key could be passed here so the renderDateCell override doesn't need to care about how to come up with its own unique key

pottedmeat · 2017-11-01T18:40:17Z

src/calendar/DatePicker.ts

+			'aria-haspopup': 'true',
+			id: `${this._idBase}_${type}_button`,
+			classes: this.classes(
+				(css as any)[`${type}Trigger`],


It would be nice to make this type safe, but I couldn't come up with a good solution that wouldn't add a ton of code.

yeah, I chose concise code over css types in this particular case, but I was kinda ambivalent about it.

pottedmeat · 2017-11-01T19:07:32Z

src/timepicker/TimePicker.ts

+	}
+
+	private _onNativeBlur(event: FocusEvent) {
+		this.properties.onBlur && this.properties.onBlur((<HTMLInputElement> event.target).value);


You could add this interface and use this instead of FocusEvent to clean things up a little.

interface FocusInputEvent extends FocusEvent { target: HTMLInputElement; }

pottedmeat · 2017-11-01T19:08:28Z

src/titlepane/TitlePane.ts

 				role: 'heading'
 			}, [
-				v('div', {
+				v('button', {


Why did this change to a button?

I always thought it should be a button, since one of the basic a11y principles in our widget design is to use native elements whenever possible. I just took this widget pattern PR as a good place to change it. It also simplifies the code a ton, since we no longer need to handle focus and key events manually.

pottedmeat · 2017-11-01T19:10:23Z

src/titlepane/TitlePane.ts

 				}, [
-					v('i', {


Why isn't this displayed anymore?

it is, I just moved it to this.renderExpandIcon()

smhigley requested review from bitpshr and pottedmeat October 27, 2017 17:14

bitpshr approved these changes Oct 31, 2017

View reviewed changes

tomdye requested changes Oct 31, 2017

View reviewed changes

smhigley added 12 commits November 2, 2017 01:58

button updates

99a9dab

calendar render udpates

17c6727

radio and checkbox render

8dc9ce6

dialog update

a7262af

slidepane

d993eaf

slider

37868af

tabs

2bb986f

textarea and textinput

0b63232

timepicker

8248715

titlepane

970071c

splitpane

dc7e02a

pr feedback

aba806b

smhigley force-pushed the simplify-render branch from 03c3dcc to aba806b Compare November 1, 2017 17:41

tomdye approved these changes Nov 1, 2017

View reviewed changes

fixed functional tests and calendar keys

b63b2a7

pottedmeat suggested changes Nov 1, 2017

View reviewed changes

pr feedback, return typings

570dcc7

smhigley merged commit bae1322 into dojo:master Nov 1, 2017

dylans added this to the 2017.11 milestone Nov 1, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplify widget render #336

Simplify widget render #336

smhigley commented Oct 27, 2017

bitpshr left a comment

bitpshr Oct 31, 2017

tomdye Oct 31, 2017

bitpshr Nov 1, 2017

bitpshr Oct 31, 2017

smhigley Nov 1, 2017 •

edited

Loading

bitpshr Oct 31, 2017

smhigley Nov 1, 2017

smhigley Nov 1, 2017

tomdye Oct 31, 2017

tomdye Oct 31, 2017

tomdye Oct 31, 2017

tomdye Oct 31, 2017

codecov bot commented Nov 1, 2017

codecov bot commented Nov 1, 2017 •

edited

Loading

pottedmeat left a comment

pottedmeat Oct 27, 2017

pottedmeat Oct 31, 2017

pottedmeat Nov 1, 2017

pottedmeat Nov 1, 2017

smhigley Nov 1, 2017

pottedmeat Nov 1, 2017

pottedmeat Nov 1, 2017

smhigley Nov 1, 2017

pottedmeat Nov 1, 2017

smhigley Nov 1, 2017

Simplify widget render #336

Simplify widget render #336

Conversation

smhigley commented Oct 27, 2017

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

smhigley Nov 1, 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

codecov bot commented Nov 1, 2017

Codecov Report

codecov bot commented Nov 1, 2017 • edited Loading

Codecov Report

pottedmeat 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

smhigley Nov 1, 2017 •

edited

Loading

codecov bot commented Nov 1, 2017 •

edited

Loading