Skip to content
This repository has been archived by the owner on Dec 12, 2018. It is now read-only.

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

Merged
merged 8 commits into from
Mar 14, 2017
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
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
60 changes: 60 additions & 0 deletions src/HeaderCell.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
import WidgetBase from '@dojo/widget-core/WidgetBase';
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

import { RegistryMixin } from '@dojo/widget-core/mixins/Registry';
import { v } from '@dojo/widget-core/d';
import { HasColumn, HasSortDetail, HasSortEvent } from './interfaces';
import { ThemeableMixin, theme, ThemeableProperties } from '@dojo/widget-core/mixins/Themeable';

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

export interface HeaderCellProperties extends ThemeableProperties, HasColumn, HasSortDetail, HasSortEvent { }
Copy link
Contributor

Choose a reason for hiding this comment

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

{ } => {}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed


export const HeaderCellBase = ThemeableMixin(RegistryMixin(WidgetBase));

@theme(headerCellClasses)
class HeaderCell extends HeaderCellBase<HeaderCellProperties> {
onSortRequest(): void {
const {
key = '',
sortDetail,
onSortRequest
} = this.properties;

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It doesn't seem like it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link

Choose a reason for hiding this comment

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

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.

});
}

render() {
const {
key,
column,
sortDetail,
onSortRequest
} = this.properties;

const classes = [ headerCellClasses.headerCell, column.sortable !== false ? headerCellClasses.sortable : null ];

const sortClasses = [
sortDetail ? headerCellClasses.sortArrow : null,
sortDetail && sortDetail.descending ? headerCellClasses.sortArrowDown : null,
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

sortDetail && !sortDetail.descending ? headerCellClasses.sortArrowUp : null
];

const onclick = (onSortRequest && (column.sortable || !column.hasOwnProperty('sortable'))) ? { onclick: this.onSortRequest } : {};
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Switched this to just column.sortable !== false


return v('th', {
role: 'columnheader',
...onclick,
classes: this.classes(...classes)
}, [
v('span', [ column.label || column.id ]),
sortDetail && sortDetail.columnId === key ? v('div', {
role: 'presentation',
classes: this.classes(...sortClasses)
Copy link
Member

Choose a reason for hiding this comment

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

Same here re building up the classes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks

}) : null
]);
}
}

export default HeaderCell;
13 changes: 13 additions & 0 deletions src/interfaces.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,8 @@
export interface SortDetails {
columnId: string;
descending?: boolean;
}

export interface ItemProperties<T> {
id: string;
data: T;
Expand All @@ -14,6 +19,14 @@ export interface HasColumn {
column: Column<any>;
}

export interface HasSortDetail {
sortDetail: SortDetails;
}

export interface HasSortEvent {
onSortRequest(sortDetail: SortDetails): void;
}

export interface HasItem {
item: ItemProperties<any>;
}
Expand Down
1 change: 1 addition & 0 deletions src/styles/dgrid.css
Original file line number Diff line number Diff line change
@@ -1 +1,2 @@
@import 'headerCell.css';
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

@import 'cell.css';
25 changes: 25 additions & 0 deletions src/styles/headerCell.css
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
.headerCell {
composes: cell from './shared/cell.css';
position: relative;
}

.sortable {
cursor: pointer;
}

.sortArrow {
position: absolute;
top: 50%;
transform: translateY(-50%);
right: 4px;
}

.sortArrowUp:after {
content: '▲';
display: block;
}

.sortArrowDown:after {
content: '▼';
display: block;
}
5 changes: 5 additions & 0 deletions src/styles/headerCell.css.d.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
export const headerCell: string;
export const sortable: string;
export const sortArrow: string;
export const sortArrowUp: string;
export const sortArrowDown: string;
Binary file added src/styles/images/ui-icons_222222_256x240.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
72 changes: 72 additions & 0 deletions tests/unit/HeaderCell.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
import * as registerSuite from 'intern/lib/interfaces/object';
import { assert } from 'chai';
import { VNode } from '@dojo/interfaces/vdom';
import HeaderCell from '../../src/HeaderCell';

registerSuite({
name: 'HeaderCell',
render: {
'renders sortable header cell with descending direction'() {
let clicked = false;
const properties = {
onSortRequest() { clicked = true; },
column: {
id: 'id',
label: 'foo',
sortable: true
},
sortDetail: {
columnId: 'id',
descending: true
},
key: 'id'
};
const headerCell = new HeaderCell();
headerCell.setProperties(properties);

const vnode = <VNode> headerCell.__render__();
vnode.properties!.onclick!.call(headerCell);

assert.strictEqual(vnode.vnodeSelector, 'th');
assert.isFunction(vnode.properties!.onclick);
assert.isTrue(clicked);
assert.equal(vnode.properties!['role'], 'columnheader');
assert.lengthOf(vnode.children, 2);
assert.equal(vnode.children![0].vnodeSelector, 'span');
assert.equal(vnode.children![0].text, 'foo');
assert.equal(vnode.children![1].vnodeSelector, 'div');
assert.equal(vnode.children![1].properties!['role'], 'presentation');
},
'renders sortable header cell with ascending direction'() {
let clicked = false;
const properties = {
onSortRequest() { clicked = true; },
column: {
id: 'id',
label: 'foo',
sortable: true
},
sortDetail: {
columnId: 'id',
descending: false
},
key: 'id'
};
const headerCell = new HeaderCell();
headerCell.setProperties(properties);

const vnode = <VNode> headerCell.__render__();
vnode.properties!.onclick!.call(headerCell);

assert.strictEqual(vnode.vnodeSelector, 'th');
assert.isFunction(vnode.properties!.onclick);
assert.isTrue(clicked);
assert.equal(vnode.properties!['role'], 'columnheader');
assert.lengthOf(vnode.children, 2);
assert.equal(vnode.children![0].vnodeSelector, 'span');
assert.equal(vnode.children![0].text, 'foo');
assert.equal(vnode.children![1].vnodeSelector, 'div');
assert.equal(vnode.children![1].properties!['role'], 'presentation');
}
}
});
1 change: 1 addition & 0 deletions tests/unit/all.ts
Original file line number Diff line number Diff line change
@@ -1 +1,2 @@
import './HeaderCell';
import './Cell';
2 changes: 1 addition & 1 deletion tslint.json
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@
"variable-declaration": "onespace"
} ],
"use-strict": false,
"variable-name": [ true, "check-format", "allow-leading-underscore", "ban-keywords" ],
"variable-name": [ true, "check-format", "allow-leading-underscore", "ban-keywords", "allow-pascal-case" ],
"whitespace": [ true, "check-branch", "check-decl", "check-operator", "check-module", "check-separator", "check-type", "check-typecast" ]
}
}