feat(oui-datagrid): add selectable rows #242

frenautvh · 2018-07-31T12:50:44Z

UK-42

FredericEspiau · 2018-07-31T14:31:47Z

packages/oui-datagrid/README.md

+### Global Actions
+
+```html:preview
+<oui-datagrid rows="$ctrl.data" page-size="5" global-actions>


Wouldn't it be best to name the parameter "selectable-rows" ?

sounds good

Yes, selectable-rows is better :)
Don't forget to add the details in the API table below for this attribute

Were it only me, I would have named it allows-row-selection but I know you guys won't like it 😄

In this way, we keep our logic of *able attributes

FredericEspiau · 2018-07-31T14:37:49Z

packages/oui-datagrid/src/datagrid.controller.js

            this.displayedRows = DatagridController.createEmptyRows(this.paging.getCurrentPageSize());
        }

+        this.selectedRows = this.selectedRows.map(() => false);


Can't this contain the actual rows instead of an array of booleans ? Or something to identify the rows ?

{ id: rowId, isSelected: false }

If we keep the array of booleans system, then it should be names something like rowSelectionStatus

since every checkbox needs to have an ng-model i still have to created N booleans for binding ; that's why it made sense to use an array of booleans (i reuse the array)

We could react to the checkbox's on-change event and add the corresponding row to the array of rows.

i think your suggestion would make the code "faster" : no reduce in getSelectedRows since you access array directly ; downside is that you need an additionnal array to store rows which is less clean (you still have to give an ngModel to checkbox so you also need an array of booleans).
did i understand you correcly ?

FredericEspiau · 2018-07-31T14:38:15Z

packages/oui-datagrid/src/datagrid.controller.js

+        }, []);
+    }
+
+    onSelectRow (index, selected) {


onRowSelection ? Still feels wrong as this method is called even when the row is unselected

onToggleSelectRow would be better i think

Yes, good name !

In this case, toggleRowSelection seems more appropriate

FredericEspiau · 2018-07-31T14:40:14Z

packages/oui-datagrid/src/datagrid.controller.js

+        }, []);
+    }
+
+    onSelectRow (index, selected) {


selected could be named isSelected, so we know it's a boolean

FredericEspiau · 2018-07-31T14:41:55Z

packages/oui-datagrid/src/datagrid.controller.js

+        } else if (selectedRowsCount === 0) {
+            this.selectAllRows = false;
+        } else {
+            this.selectAllRows = null;


Is this the global comportement for tri-select box ? null is some items are selected, some are not? Because, if it's not already the selected behavior, this would require a something else than a boolean then

yep checkbox component currently use "null"

FredericEspiau · 2018-07-31T14:42:51Z

packages/oui-datagrid/src/datagrid.controller.js

+        }
+    }
+
+    toggleSelectAllRows (modelValue) {


currentSelectionStatus ?

We could keep the same logic as above, toggleAllRowsSelection

FredericEspiau · 2018-07-31T14:44:13Z

@frenautvh I've only commented because I feel most point would require a discussion

AxelPeter

Some improvements needed, but seems good.
Don't forget the unit tests and the documentation :)

AxelPeter · 2018-07-31T15:20:25Z

packages/oui-datagrid/README.md

+### Global Actions
+
+```html:preview
+<oui-datagrid rows="$ctrl.data" page-size="5" global-actions>


Yes, selectable-rows is better :)
Don't forget to add the details in the API table below for this attribute

AxelPeter · 2018-07-31T15:23:15Z

packages/oui-datagrid/src/cell/cell.controller.js

            this._compileCell();
        }
+
+        this.index = this.index || 0;


Should be in $onInit

AxelPeter · 2018-07-31T15:35:12Z

packages/oui-datagrid/src/datagrid.controller.js

+        }, []);
+    }
+
+    onSelectRow (index, selected) {


In this case, toggleRowSelection seems more appropriate

AxelPeter · 2018-07-31T15:38:52Z

packages/oui-datagrid/src/datagrid.controller.js

+        }
+    }
+
+    toggleSelectAllRows (modelValue) {


We could keep the same logic as above, toggleAllRowsSelection

AxelPeter

@frenautvh From the staging, the first column is not fixed. Can you check it ?
Caution on your CDS builds too :)

JeremyDec · 2018-08-02T15:03:01Z

packages/oui-datagrid/src/cell/cell.controller.js

        }
+
+        this.cellScope.$watch(() => this.datagridCtrl.selectedRows[this.index], (isSelected) => {
+            this.cellScope.$isSelected = isSelected || false;


this.cellScope.$isSelected = !!isSelected is possible no ?

!! is harder to read by someone not used to this js "trick" but yes it would fit the job

frenautvh · 2018-08-02T15:35:48Z

@antleblanc @euhmeuh @FredericEspiau
Thanks for your comments, i know that there is more than one way to do one thing. What i don't know is why you choose your way over mine and that's the interesting part so please tell me why when you're suggesting alternatives! :) Otherwise we could comment every line of code and never make a change because everyone is right

AxelPeter · 2018-08-03T08:26:42Z

@frenautvh
The right way is the way that works and that keeps the same logic in the project (naming, structure, etc.). That's why I'm here to take the lead on those decisions.
You could still add comments on your choices to explain why you do it that way, and why it's the better choice in your opinion ;)

@antleblanc @euhmeuh @FredericEspiau @JeremyDec
Can you take a position on your review ?
Without approve or request changes label, we can't know your position on your comments. Even if it's for discussing :)
For comments that are not really important, or that haven't an impact on your review, I propose you to add a label (optional) before each comment.

frenautvh · 2018-08-03T08:44:40Z

@AxelPeter I'm targeting that kind of comments :

"Can't this contain the actual rows instead of an array of booleans ? Or something to identify the rows ?"
"this.cellScope.$isSelected = !!isSelected is possible no ?"
"We could react to the checkbox's on-change event and add the corresponding row to the array of rows."
I know we could do that but WHY would we? I think the reviewer should at least gives an hint otherwise it's just noise in the PR review.

Example :

"We could react to the checkbox's on-change event because in order to avoids to generate array of row each time the binding is updated"
"You could do this.cellScope.$isSelected = !!isSelected; i think the syntax is more readable"

AxelPeter · 2018-08-03T08:54:25Z

I know and I agree, that's why I suggest some methods to everybody for working together (it works with our previous team) ;)

feat(oui-datagrid): add global actions

74453a6

frenautvh added the feature label Jul 31, 2018

frenautvh requested review from AxelPeter, FredericEspiau, antleblanc and euhmeuh July 31, 2018 12:50

FredericEspiau reviewed Jul 31, 2018

View reviewed changes

AxelPeter requested a review from JeremyDec July 31, 2018 14:48

AxelPeter suggested changes Jul 31, 2018

View reviewed changes

frenautvh added 2 commits August 1, 2018 14:30

feat(oui-datagrid): global actions code review changes

864d05d

feat(oui-datagrid): add selectable-rows unit tests

7159d24

AxelPeter suggested changes Aug 2, 2018

View reviewed changes

AxelPeter approved these changes Aug 2, 2018

View reviewed changes

frenautvh added the quality check label Aug 2, 2018

AxelPeter added quality check ok and removed quality check labels Aug 2, 2018

JeremyDec reviewed Aug 2, 2018

View reviewed changes

ovh-ux deleted a comment from ovh-cds Aug 3, 2018

AxelPeter changed the title ~~feat(oui-datagrid): add global actions~~ feat(oui-datagrid): add selectable rows Aug 3, 2018

AxelPeter merged commit 08d0841 into develop Aug 3, 2018

AxelPeter deleted the feat/datagrid-global-actions branch August 3, 2018 12:57

AxelPeter mentioned this pull request Aug 3, 2018

Docs/standardization #243

Merged

feat(oui-datagrid): add selectable rows #242

feat(oui-datagrid): add selectable rows #242

Uh oh!

Conversation

frenautvh commented Jul 31, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AxelPeter Jul 31, 2018 • 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!

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!

AxelPeter Jul 31, 2018 • 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!

FredericEspiau commented Jul 31, 2018

Uh oh!

AxelPeter left a comment

Choose a reason for hiding this comment

Uh oh!

AxelPeter Jul 31, 2018 • 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!

AxelPeter Jul 31, 2018 • 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!

AxelPeter left a comment • 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!

frenautvh commented Aug 2, 2018

Uh oh!

AxelPeter commented Aug 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

frenautvh commented Aug 3, 2018

Uh oh!

AxelPeter Jul 31, 2018 •

edited

Loading

AxelPeter Jul 31, 2018 •

edited

Loading

AxelPeter Jul 31, 2018 •

edited

Loading

AxelPeter Jul 31, 2018 •

edited

Loading

AxelPeter left a comment •

edited

Loading

AxelPeter commented Aug 3, 2018 •

edited

Loading

AxelPeter commented Aug 3, 2018 •

edited

Loading