Header cell with unit tests, styling, and new interfaces #8

pottedmeat · 2017-02-24T04:25:47Z

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:

Adds the HeaderCell widget with styling and unit tests. Adds interfaces used by the HeaderCell properties.

codecov-io · 2017-02-24T04:35:00Z

Codecov Report

Merging #8 into master will increase coverage by 13.35%.
The diff coverage is 78.57%.

@@             Coverage Diff             @@
##           master       #8       +/-   ##
===========================================
+ Coverage   51.72%   65.07%   +13.35%     
===========================================
  Files           4        6        +2     
  Lines          29       63       +34     
  Branches        1        9        +8     
===========================================
+ Hits           15       41       +26     
- Misses         14       16        +2     
- Partials        0        6        +6

Impacted Files	Coverage Δ
src/HeaderCell.ts	`78.57% <78.57%> (ø)`
_build/src/styles/headerCell.css.js	`66.66% <0%> (ø)`

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 759acd7...e20b1e2. Read the comment docs.

rishson · 2017-02-28T12:04:34Z

src/HeaderCell.ts

+export interface HeaderCellProperties extends ThemeableProperties, HasColumn, HasSortDetail, HasSortEvent { }
+
+@theme(headerCellClasses)
+class HeaderCell extends ThemeableMixin(RegistryMixin(WidgetBase))<HeaderCellProperties> {


base type as const - same as row?

rishson · 2017-02-28T12:06:03Z

src/styles/headerCell.css

+.sortArrow {
+    width: 16px;
+    height: 16px;
+    background-image: url('images/ui-icons_222222_256x240.png');


Can we just set content to use a unicode arrow char (like ComboBox does) for now?

tomdye · 2017-02-28T12:39:59Z

src/HeaderCell.ts

+			sortDetail
+		} = this.properties;
+
+		const classes = [ headerCellClasses.cell ];


You could do a ternary here and return null for sortable = false. Classes function deals with that

Excellent. Thanks

tomdye · 2017-02-28T12:40:42Z

src/HeaderCell.ts

+			v('span', [ column.label || column.id ]),
+			sortDetail && sortDetail.columnId === key ? v('div', {
+				role: 'presentation',
+				classes: this.classes(...sortClasses)


Same here re building up the classes

tomdye · 2017-02-28T14:01:30Z

src/styles/headerCell.css

+.sortArrow {
+    width: 16px;
+    height: 16px;
+    background-image: url('images/ui-icons_222222_256x240.png');


I don't think we should be adding this png sprite file. We should use unicode as @rishson suggested until we've created an icon widget / chosen an icon approach for widgets

tomdye · 2017-02-28T14:02:29Z

src/styles/dgrid.css

@@ -1 +1,2 @@
+@import 'headerCell.css';


if you're going to offer up a dgrid.css file for people using dgrid outside of our cli-webpack build then you need to ensure you add an exclusion rule for it in your gruntfile otherwise it will be built into a separate css-module and the generated classes will mean zero.

I'm not sure how to exclude this from the poscss task. Right now I'm using the pattern from dojo/widgets that just overwrites the file with its original version.

agubler · 2017-03-07T13:43:52Z

src/HeaderCell.ts

+			onSortRequest
+		} = this.properties;
+
+		if (onSortRequest && (column.sortable || !column.hasOwnProperty('sortable'))) {


do you think it would be better to not attach the handler if the column is not sortable?

maier49

Looks good to me, just one question.

maier49 · 2017-03-10T17:33:45Z

src/HeaderCell.ts

+
+		onSortRequest({
+			columnId: key,
+			descending: Boolean(sortDetail && sortDetail.columnId === key && !sortDetail.descending)


descending is getting set to !sortDetail.descending, is that correct?

It doesn't seem like it

No, this is correct. It'll reverse the current sort direction

In that case the naming seems a little confusing at first glance. Maybe I just need to look at the overall code a bit more for it to make sense.

agubler · 2017-03-10T17:46:14Z

src/HeaderCell.ts

+			sortDetail && !sortDetail.descending ? headerCellClasses.sortArrowUp : null
+		];
+
+		const onclick = (onSortRequest && (column.sortable || !column.hasOwnProperty('sortable'))) ? { onclick: this.onSortRequest } : {};


This reads really difficultly, I think worked it out that !column.hasOwnProperty('sortable') is effectively saying all columns are sortable by default?

I think we can split this out to make it clearer then.

Switched this to just column.sortable !== false

msssk · 2017-03-10T18:09:28Z

src/HeaderCell.ts

@@ -0,0 +1,60 @@
+import WidgetBase from '@dojo/widget-core/WidgetBase';


import order
https://github.com/dojo/meta/blob/master/STYLE.md#ordering

msssk · 2017-03-10T18:11:26Z

src/HeaderCell.ts

+
+import * as headerCellClasses from './styles/headerCell.css';
+
+export interface HeaderCellProperties extends ThemeableProperties, HasColumn, HasSortDetail, HasSortEvent { }


{ } => {}

msssk · 2017-03-10T22:36:47Z

src/HeaderCell.ts

+
+		const sortClasses = [
+			sortDetail ? headerCellClasses.sortArrow : null,
+			sortDetail && sortDetail.descending ? headerCellClasses.sortArrowDown : null,


I think clarity is retained if this line and the following are collapsed into
sortDetail && sortDetail.descending ? headerCellClasses.sortArrowDown : headerClasses.sortArrowUp

Since these all depend on sortDetail, I did a ternary that either creates this array or an empty array, then used your ternary approach. It does read much better. Thanks.

msssk

Several modules could be improved by using consistent import order per the style guide:
https://github.com/dojo/meta/blob/master/STYLE.md#ordering

An issue not specific to this PR:
I'm not convinced of the need for an id property on columns. It seems like a confusing middle-man for field. If 99% of the time field is just going to end being unspecified and defaulting to id, then let's drop id and use field instead. The 1% of cases where the distinction between id and field matters can be handled without complicating the 99% of cases where it does not matter.

agubler · 2017-03-15T00:28:57Z

@MSSK how do you plan on supporting the 1% where the distinction between field and id does matter?

msssk · 2017-03-15T00:33:23Z

@agubler The same way dgrid always has: you specify a column definition with an arbitrary field name that doesn't correspond to a field in the data, and provide a get method that returns the desired value. And honestly, it's probably well under 1% of the time that someone might want to do this. I can't even recall ever seeing it in ~5 years of using dgrid - anyone else?

agubler · 2017-03-15T00:37:39Z

That would assume the same dgrid1 mechanisms, I just don't want to get to a position where something cannot be supported because there was the consensus that it would never need to be.

msssk · 2017-03-15T00:49:16Z

I'm optimistic that #12 will be resolved in a way that provides similar mechanisms to dgrid 1. We definitely want to maintain flexibility and extensibility. I'm just proposing a way of achieving that while keeping the API simple and user-friendly.

Header cell with unit tests, styling, and new interfaces

23008b2

pottedmeat requested review from rishson, agubler and matt-gadd February 24, 2017 04:25

dylans added this to the 2017.02 milestone Feb 24, 2017

rishson reviewed Feb 28, 2017

View reviewed changes

tomdye suggested changes Feb 28, 2017

View reviewed changes

Make the mixed in class a type

57ab9ad

dylans modified the milestones: 2017.02, 2017.03 Mar 1, 2017

pottedmeat added 2 commits March 6, 2017 10:46

Updating theming

c657fd2

Update lint rules to allow typing workaround

0a2542b

agubler reviewed Mar 7, 2017

View reviewed changes

Dynamically add onclick event

a28e905

maier49 approved these changes Mar 10, 2017

View reviewed changes

agubler reviewed Mar 10, 2017

View reviewed changes

msssk reviewed Mar 10, 2017

View reviewed changes

msssk suggested changes Mar 10, 2017

View reviewed changes

pottedmeat added 3 commits March 12, 2017 16:28

Order imports

4965fd0

Integrate feedback

d4e1d5c

Type updates

e20b1e2

pottedmeat merged commit 937b80a into dojo:master Mar 14, 2017

pottedmeat deleted the initial-header-cell branch March 14, 2017 19:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Header cell with unit tests, styling, and new interfaces #8

Header cell with unit tests, styling, and new interfaces #8

pottedmeat commented Feb 24, 2017

codecov-io commented Feb 24, 2017 •

edited by codecov bot

Loading

rishson Feb 28, 2017

pottedmeat Feb 28, 2017

rishson Feb 28, 2017

tomdye Feb 28, 2017

pottedmeat Feb 28, 2017

tomdye Feb 28, 2017

pottedmeat Feb 28, 2017

tomdye Feb 28, 2017

tomdye Feb 28, 2017

pottedmeat Feb 28, 2017

agubler Mar 7, 2017

maier49 left a comment

maier49 Mar 10, 2017

pottedmeat Mar 10, 2017

pottedmeat Mar 10, 2017

maier49 Mar 10, 2017

agubler Mar 10, 2017

pottedmeat Mar 10, 2017

agubler Mar 10, 2017

pottedmeat Mar 12, 2017

msssk Mar 10, 2017

pottedmeat Mar 12, 2017

msssk Mar 10, 2017

pottedmeat Mar 12, 2017

msssk Mar 10, 2017

pottedmeat Mar 12, 2017

msssk left a comment

agubler commented Mar 15, 2017

msssk commented Mar 15, 2017

agubler commented Mar 15, 2017

msssk commented Mar 15, 2017

		@@ -0,0 +1,60 @@
		import WidgetBase from '@dojo/widget-core/WidgetBase';


		import * as headerCellClasses from './styles/headerCell.css';

		export interface HeaderCellProperties extends ThemeableProperties, HasColumn, HasSortDetail, HasSortEvent { }

Header cell with unit tests, styling, and new interfaces #8

Header cell with unit tests, styling, and new interfaces #8

Conversation

pottedmeat commented Feb 24, 2017

codecov-io commented Feb 24, 2017 • edited by codecov bot 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

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

maier49 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

msssk left a comment

Choose a reason for hiding this comment

agubler commented Mar 15, 2017

msssk commented Mar 15, 2017

agubler commented Mar 15, 2017

msssk commented Mar 15, 2017

codecov-io commented Feb 24, 2017 •

edited by codecov bot

Loading