Row with unit tests, styling, and interfaces #7

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

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 Row widget with styling and unit tests. Adds interfaces used by the Row properties.

codecov-io · 2017-02-24T04:30:25Z

Codecov Report

Merging #7 into master will increase coverage by 8.27%.
The diff coverage is 100%.

@@           Coverage Diff           @@
##           master    #7      +/-   ##
=======================================
+ Coverage   51.72%   60%   +8.27%     
=======================================
  Files           4     7       +3     
  Lines          29    55      +26     
  Branches        1     3       +2     
=======================================
+ Hits           15    33      +18     
- Misses         14    22       +8

Impacted Files	Coverage Δ
src/Row.ts	`100% <100%> (ø)`
_build/src/styles/shared/rowTable.css.js	`0% <0%> (ø)`
_build/src/styles/row.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...810bea6. Read the comment docs.

agubler · 2017-02-24T17:48:53Z

src/Row.ts

+}
+
+@theme(rowClasses)
+class Row  extends ThemeableMixin(RegistryMixin(WidgetBase))<RowProperties> {


When using mixins we need to declare the base type as a const before using it to extend.

export type RowBase = ThemeableMixin(RegistryMixin(WidgetBase)); class Row extends RowBase<RowProperties> { // ..... }

Also there is an extra space.

Is there more info on what causes this?

What do you mean by an extra space?

There's lots of info in the dojo 2 room and I also raised an issue on typescript, I don't have the link to hand (I'll add later).

Basically you cannot extend an expression, which Mixin(Class)<Properties> actually is. With the release of 2.2.1 this would actually fail to create the d.ts file when running grunt dist.

There's an extra space after Row and before extends... just killing my OCD.

Ahh, cool. Yeah, I hate extra spaces too just was blind to it.

agubler · 2017-02-24T17:49:35Z

tests/unit/Row.ts

+			item: { foo: 'bar' }
+		};
+
+		const rowVie = new Row();


What is a rowVie?

tomdye · 2017-02-28T12:36:29Z

src/styles/dgrid.css

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


I don't think this file is needed. Unless you add an exception to the grunt-dojo2 rules for this project this will be built into a css-module file but the generated class names will not line up with those used on the widgets.

tomdye · 2017-02-28T12:37:23Z

src/styles/row.css

+}
+
+.rowTable {
+    composes: rowTable from './shared/rowTable.css';


Why does rowtable need to be in a separate css file? Is it used elsewhere?

Yes, it's also used in the header

agubler · 2017-03-07T13:39:20Z

src/Row.ts

+				classes: this.classes(rowClasses.rowTable)
+			}, [
+				v('tr', columns.map(({ id, field }) => {
+					return w('cell', <CellProperties> {


Do you need to cast here?

I think this is the only way to ensure I obey that interface

Can you not do w<CellProperties>('cell', { ... })?

maier49 · 2017-03-10T17:50:58Z

src/Row.ts

+import * as rowClasses from './styles/row.css';
+
+export interface RowProperties extends WidgetProperties, HasColumns, RegistryMixinProperties {
+	item: any;


Could there be any benefit from making this generic, or at least providing a base type? It looks like the code is assuming that item.data will exist for example.

The field properties in the column definitions could even be required to be a member of keyof ItemDataType or something.

Good catch. I like the field stuff as well. I'm going to make a note to go through and see where I can refine that.

msssk · 2017-03-10T22:44:40Z

src/Row.ts

@@ -0,0 +1,47 @@
+import { WidgetProperties } from '@dojo/widget-core/interfaces';


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

msssk · 2017-03-10T22:49:56Z

tests/support/util.ts

@@ -16,3 +19,19 @@ export function isEventuallyRejected<T>(promise: Thenable<T>): Thenable<boolean>
 export function throwImmediatly() {


throwImmediatly => throwImmediately

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

pottedmeat added 2 commits February 23, 2017 22:17

Row with unit tests, styling, and new interfaces

ab5353f

Add row to CSS imports file

7af316b

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

agubler suggested changes Feb 24, 2017

View reviewed changes

pottedmeat added 2 commits February 24, 2017 14:43

Update style and add mixin typing workaround

9071252

Fixing the “rowVie” variable one more time

37808ce

tomdye suggested changes Feb 28, 2017

View reviewed changes

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

agubler reviewed Mar 7, 2017

View reviewed changes

maier49 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 4 commits March 12, 2017 16:49

Fix spelling mistake

44948cc

Fix import order. Properly type item property.

2962bcd

Type updates

810bea6

Merge branch 'master' into initial-row

a3665a7

pottedmeat merged commit 6927258 into dojo:master Mar 14, 2017

pottedmeat deleted the initial-row branch March 14, 2017 19:32

		@@ -0,0 +1,47 @@
		import { WidgetProperties } from '@dojo/widget-core/interfaces';

		@@ -16,3 +19,19 @@ export function isEventuallyRejected<T>(promise: Thenable<T>): Thenable<boolean>
		export function throwImmediatly() {

Row with unit tests, styling, and interfaces #7

Row with unit tests, styling, and interfaces #7

Uh oh!

Conversation

pottedmeat commented Feb 24, 2017

Uh oh!

codecov-io commented Feb 24, 2017 • edited by codecov bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

agubler Feb 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tomdye Feb 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

msssk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov-io commented Feb 24, 2017 •

edited by codecov bot

Loading

agubler Feb 24, 2017 •

edited

Loading

tomdye Feb 28, 2017 •

edited

Loading