Customize cell content, fixes #12 #37

pottedmeat · 2017-05-22T18:31:04Z

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 new callbacks to the Column interface, updates Row to use the new Column definition to find Cell content, and adds unit tests to check the new behavior.

codecov · 2017-05-22T18:40:38Z

Codecov Report

Merging #37 into master will increase coverage by 0.07%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master      #37      +/-   ##
==========================================
+ Coverage   98.17%   98.24%   +0.07%     
==========================================
  Files          10       10              
  Lines         219      228       +9     
  Branches       34       36       +2     
==========================================
+ Hits          215      224       +9     
  Partials        4        4

Impacted Files	Coverage Δ
src/Cell.ts	`100% <100%> (ø)`	⬆️
src/providers/ArrayDataProvider.ts	`100% <100%> (ø)`	⬆️
src/Row.ts	`100% <100%> (ø)`	⬆️

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 4cbb547...00713f1. Read the comment docs.

dylans

For the things I review for typically, this looks fine, though someone else should review and approve before it lands. Thanks!

msssk · 2017-05-24T22:56:56Z

src/interfaces.d.ts

-export interface Column<T> {
+import { DNode } from '@dojo/widget-core/interfaces';
+
+export interface Column<T = any, V = string> {


I still don't see the need for id - field is enough.

msssk · 2017-05-24T23:04:23Z

src/interfaces.d.ts

 }

 export interface HasValue {
 	value: string;
 }

-export interface ItemProperties<T> {
+export interface ItemProperties<T = any> {


Am I understanding correctly that dgrid requires data objects to look like this:

{ id: <idValue>, data: { <... data properties> } }

Are we not able to continue dgrid 1's design of accepting flatter objects (with support for idProperty as well):

{ id: <idValue>, <... data properties> }

Right now, the object structure is simple, but ItemProperties adds much more moving forward.

I think it would be very good if dgrid 2 is able to continue supporting data in the same format as dgrid 1 unless there's a compelling reason not to - is there?

While Dojo's data solutions have evolved over the years, the basic format of a data object has been consistent for years. What is the compelling need to change it?

Also - considering dgrid 2 requires a data provider, are my concerns relevant? Can one store be used for consumers that like the old data format, and also for dgrid, with the differences implemented in the data provider?

This doesn't really have anything to do with the data - it's what the data provider gives to the grid. ArrayDataProvider still supports simple data with idProperty.

msssk · 2017-05-24T23:08:17Z

src/Cell.ts

 import { theme, ThemeableMixin, ThemeableProperties } from '@dojo/widget-core/mixins/Themeable';

 import * as cellCss from './styles/shared/cell.m.css';
 import * as css from './styles/cell.m.css';

-export interface CellProperties extends HasColumn, HasItem, HasValue, RegistryMixinProperties, ThemeableProperties {
+export interface CellProperties extends HasColumn, HasContent, HasItem, HasValue, RegistryMixinProperties, ThemeableProperties {


What is the purpose of HasValue for Cell? It's not used in the rendering... is it envisioned that one might want to read the value of value from a Cell instance?

That's my thought, yes.

agubler

A few initial comments

agubler · 2017-05-25T12:11:45Z

src/interfaces.d.ts

-export interface Column<T> {
+import { DNode } from '@dojo/widget-core/interfaces';
+
+export interface Column<T = any, V = string> {


We should have some documentation for exported interfaces, will help explain the intent of the properties and functions.

agubler · 2017-05-25T12:16:16Z

src/Row.ts

+					}
+
+					let content: DNode;
+					if (column.render) {


I'm not sure I get why we need render and renderValue, couldn't there just be a render (perhaps a better name, to not conflate with a widgets render function) and just have an optional third parameter value?: V?

I've been going back and forth on this, and it's been discussed in the referenced issue. My intent with render was to have a function that indicates it will not be consuming any data - adding an action button, for example. Absent this, we will still attempt to read a value from the underlying data - which isn't horrible but creates a side-effect that may be unexpected and could (though it would be unlikely) create problems.

Some alternatives I'd been considering:

Add something like an enum that can be assigned to field that would indicate no value should be read from the data object

Allow get to be either a function or a value - perhaps rename this property to value

What do you think of these alternatives?

I don't fully understand the problem with having a single function but with an API that passes the value field, this leaves it up to the consumer to decide how to implement the customisation and choose to use the value or not.

The most general use-case in our experience is having a custom get and a custom renderValue. In this situation, the user will probably only care about the value and not the item or column, so having value first would be ideal. If they don't care about having a value, having value last would be ideal. I'm messing around with this signature and I'm not 100% happy with having value last.

Then do you think it would be more ergonomic to have an options style API?

interface CustomRenderOptions { item: any; column: Column; value: any; } // consumer implements function and only uses what they need from the options function customRender(options: CustomRenderOptions): DNode { // do something with either options.item, options.column or options.value // or any combination. }

I am still not sure about needing get to be a function when the existing render function can be used for that.

I had considered the options API, but with this covering the most common use cases in the first arguments, it doesn't seem worth the overhead.

Do you have any suggestions for a better name?

render may be used without get and get may be used without render. Combining everything into a single function will tightly couple the customization our users have traditionally done as well as force users to implement what are pretty standard defaults.

With the options I'd just expect consumers to destructure what is needed at the top of their custom renderer function (which I think is ergonomic).

const { value } = options;

Both get and render return a DNode and are used to effectively populate content (i'm not sure why cell needs value now there is content) that is used by Cell? Just feels like having two functions that end up as the same thing - content is more confusing than flexible (would be for me), because I can return a string either based on item and column.

So just having a API for customCellRenderer(item: any, column: Column): DNode {}, should cover all the scenarios needed?

const { customCellRenderer, field = column.id } = column; let content = item.data[ field ] ? String(item.data[ field ]) : ''; if (customCellRenderer) { content = customCellRenderer(item, column); }

Means a consumer can return a

Static string:

function customCellRenderer(item: any, column: Column): DNode { return 'static-string'; }

Dynamic string based on the item

function customCellRenderer(item: any, column: Column): DNode { const { field = column.id } = column; const value = item[field]; return `Miss ${value}`; }

A custom DNode

function customCellRenderer(item: any, column: Column): DNode { return v('button', { /* some properties */}); }

Unless I've missed a scenario that having the two separate functions covers.

get is specific to finding the value to be used in a cell (the combination of the item and column) and is meant to present an alternative to the default method of retrieving this value from the item based on a field.

render is meant to take this value and turn it into the UI that will be used inside the cell. render is reusable, whereas get is not. So render could appear across all columns, while get is unique to each column.

I made it so that get could return a DNode so that if someone really wanted to do everything in a single function, they could (though I agree it isn't great practice, it just seemed the best way to do things if we wanted to avoid having separate render methods for when you do and do not care about the cell's value).

I hope this is enough to explain why get and render, value and content are two separate ideas that seem to encourage separate levels of customization. If that can be agreed upon, I can remove the DNode return from get so it retains this clear separation and I guess I can also make the render function accept an object, though I wish there was a way to do this without so much overhead.

agubler · 2017-05-25T12:21:21Z

src/Row.ts

-					const { id, field } = column;
+					const { field, id } = column;
+
+					let value: any;


Perhaps would be nicer it could be simplified a bit (does assume consolidating the two render functions):

let value = item.data[ field || id] ? String(item.data[ field || id ]) : ''; if (column.get) { value = column.get(item, column); } let content: DNode = value; if (column.render) { content = column.render(item, column, value); }

kitsonk · 2017-05-26T08:29:51Z

src/interfaces.d.ts

+ * @property  get      Manually return the value or content to use for this column
+ * @property  render   Use this column's value to provide content for this column
+ */
+export interface Column<T = any, V = string> {


Alphabetise please... :-D

I think I'm reading the style guide on interface property ordering correctly - listing properties then methods. I couldn't find anything on ordering documentation for interfaces but is that what you're referring to here that needs alphebetized?

yes, we need to make it explicit, we have been trying to alphabetize things by default, as when they get added to, it becomes difficult to find stuff in big interefaces... properties then methods, yup, but alphabetical within those groups

Is it not alphabetized within each group right now?

get is listed as a property with a value of method (or a string), where render is an optional method... not the same class of properties

Customize cell content, fixes dojo#12

Customize cell content, fixes dojo#12

271cad8

pottedmeat added the enhancement label May 22, 2017

pottedmeat requested review from dylans, msssk, kitsonk, agubler and matt-gadd May 22, 2017 18:31

dylans reviewed May 22, 2017

View reviewed changes

kitsonk approved these changes May 23, 2017

View reviewed changes

dylans added this to the 2017.05 milestone May 24, 2017

msssk approved these changes May 24, 2017

View reviewed changes

agubler reviewed May 25, 2017

View reviewed changes

pottedmeat added 2 commits May 25, 2017 12:35

Rearrange get/render to simplify functions

f422d2f

Move render to use options, remove DNode from get return

39e6514

kitsonk reviewed May 26, 2017

View reviewed changes

Realphebetize get

00713f1

kitsonk approved these changes May 31, 2017

View reviewed changes

eheasley modified the milestones: 2017.05, 2017.06 Jun 6, 2017

pottedmeat merged commit 1be4f21 into dojo:master Jun 6, 2017

pottedmeat deleted the feature-cell-customization branch June 6, 2017 21:20

pottedmeat added a commit to pottedmeat/dgrid that referenced this pull request Jun 6, 2017

Customize cell content, fixes dojo#12 (dojo#37)

5334c34

Customize cell content, fixes dojo#12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Customize cell content, fixes #12 #37

Customize cell content, fixes #12 #37

pottedmeat commented May 22, 2017

codecov bot commented May 22, 2017 •

edited

Loading

dylans left a comment

msssk May 24, 2017

msssk May 24, 2017

pottedmeat May 25, 2017

msssk May 25, 2017

pottedmeat May 25, 2017

msssk May 25, 2017

msssk May 24, 2017

pottedmeat May 25, 2017

agubler left a comment

agubler May 25, 2017

agubler May 25, 2017

pottedmeat May 25, 2017

agubler May 25, 2017

pottedmeat May 25, 2017 •

edited

Loading

agubler May 25, 2017

agubler May 25, 2017 •

edited

Loading

pottedmeat May 25, 2017

agubler May 25, 2017

agubler May 25, 2017 •

edited

Loading

pottedmeat May 25, 2017

agubler May 25, 2017

kitsonk May 26, 2017

pottedmeat May 26, 2017

kitsonk May 26, 2017

pottedmeat May 26, 2017

kitsonk May 30, 2017

Customize cell content, fixes #12 #37

Customize cell content, fixes #12 #37

Conversation

pottedmeat commented May 22, 2017

codecov bot commented May 22, 2017 • edited Loading

Codecov Report

dylans 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

agubler 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

pottedmeat May 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agubler May 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agubler May 25, 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 May 22, 2017 •

edited

Loading

pottedmeat May 25, 2017 •

edited

Loading

agubler May 25, 2017 •

edited

Loading

agubler May 25, 2017 •

edited

Loading