[DT] Selection interface for DataTable #1195

Open
wants to merge 17 commits into
from

Conversation

Projects
None yet
3 participants
@apipkin
Contributor

apipkin commented Sep 16, 2013

Addition of row, column and cell selection. Adds the ability to use shift and control (meta) keys to select a range or toggle individual items.

Uses public method for procedural item selection and event delegation to call the procedural calls.

Has a single selection mode for row, column or cell. Switching the selection mode will clear any current selection.

  • Test
  • Code Fix
  • History
  • Sign Off or 3 days

@ghost ghost assigned apipkin Sep 16, 2013

+ -webkit-transition: background-color 0.1s ease-out;
+ -moz-transition: background-color 0.1s ease-out;
+ -o-transition: background-color 0.1s ease-out;
+ transition: background-color 0.1s ease-out;

This comment has been minimized.

Show comment Hide comment
@ericf

ericf Sep 25, 2013

Owner

Why are the timings different? Can you even notice a 0.05s transition?

@ericf

ericf Sep 25, 2013

Owner

Why are the timings different? Can you even notice a 0.05s transition?

This comment has been minimized.

Show comment Hide comment
@apipkin

apipkin Sep 25, 2013

Contributor

If you look at http://jsbin.com/UhOLeGI/1/edit

You can see the second list (veggies) has more ghosting than the first list. I didn't like that but I wanted something.

@apipkin

apipkin Sep 25, 2013

Contributor

If you look at http://jsbin.com/UhOLeGI/1/edit

You can see the second list (veggies) has more ghosting than the first list. I didn't like that but I wanted something.

src/datatable/js/select.js
+ */
+ // TODO: Maybe calculate this when `selectMode` is set and after the body
+ // is rendered
+ _rowIndexOffset: -2,

This comment has been minimized.

Show comment Hide comment
@ericf

ericf Sep 25, 2013

Owner

What's this?

@ericf

ericf Sep 25, 2013

Owner

What's this?

This comment has been minimized.

Show comment Hide comment
@apipkin

apipkin Sep 25, 2013

Contributor

This is to offset the rowIndex of the first tbody row. With a header and a message tbody, the first row of the data has an index of 2 when it should be zerol

@apipkin

apipkin Sep 25, 2013

Contributor

This is to offset the rowIndex of the first tbody row. With a header and a message tbody, the first row of the data has an index of 2 when it should be zerol

This comment has been minimized.

Show comment Hide comment
@ericf

ericf Sep 25, 2013

Owner

If this is used per-instance the value should not be defined here.

@ericf

ericf Sep 25, 2013

Owner

If this is used per-instance the value should not be defined here.

This comment has been minimized.

Show comment Hide comment
@Satyam

Satyam Sep 26, 2013

Contributor

This offset is used in many places, just search for rowIndex in the datatable source files and you'll find several instances. I used it in keynav.js and in editable.js. It must be calculated since it can change, for example, with nested headers. It might even changed dynamically by the developer adding thead sections or rows to existing ones. If so, it might be better calculated on the fly like this:

rowIndexOffset = tbody.get('firstChild.rowIndex');
@Satyam

Satyam Sep 26, 2013

Contributor

This offset is used in many places, just search for rowIndex in the datatable source files and you'll find several instances. I used it in keynav.js and in editable.js. It must be calculated since it can change, for example, with nested headers. It might even changed dynamically by the developer adding thead sections or rows to existing ones. If so, it might be better calculated on the fly like this:

rowIndexOffset = tbody.get('firstChild.rowIndex');

This comment has been minimized.

Show comment Hide comment
@apipkin

apipkin Sep 26, 2013

Contributor

I'll make that change

@apipkin

apipkin Sep 26, 2013

Contributor

I'll make that change

src/datatable/js/select.js
+ @type Object
+ @since @SINCE@
+ */
+ _selectDelegate: null,

This comment has been minimized.

Show comment Hide comment
@ericf

ericf Sep 25, 2013

Owner

I don't like the idea of storing per-instance state on the prototype. I'd remove the code but leave the docs for this.

@ericf

ericf Sep 25, 2013

Owner

I don't like the idea of storing per-instance state on the prototype. I'd remove the code but leave the docs for this.

This comment has been minimized.

Show comment Hide comment
@apipkin

apipkin Sep 25, 2013

Contributor

Where would you place this? Or do you mean, don't define it on the prototype directly until it's set?

@apipkin

apipkin Sep 25, 2013

Contributor

Where would you place this? Or do you mean, don't define it on the prototype directly until it's set?

This comment has been minimized.

Show comment Hide comment
@ericf

ericf Sep 25, 2013

Owner

Don't define the property on the prototype at all. Just have the API doc block, and assign the values in the initializer.

@ericf

ericf Sep 25, 2013

Owner

Don't define the property on the prototype at all. Just have the API doc block, and assign the values in the initializer.

src/datatable/js/select.js
+ @default null
+ @since @SINCE@
+ */
+ _lastSelectIndex: null,

This comment has been minimized.

Show comment Hide comment
@ericf

ericf Sep 25, 2013

Owner

Ditto.

@ericf

ericf Sep 25, 2013

Owner

Ditto.

src/datatable/js/select.js
+ @type Object
+ @since @SINCE@
+ */
+ _selectSelected: [],

This comment has been minimized.

Show comment Hide comment
@ericf

ericf Sep 25, 2013

Owner

Ditto, especially this one!

@ericf

ericf Sep 25, 2013

Owner

Ditto, especially this one!

src/datatable/js/select.js
+ */
+ selectAll: function () {
+ var tbody = this.body.tbodyNode,
+ mode = this.get('selectMode'),

This comment has been minimized.

Show comment Hide comment
@ericf

ericf Sep 25, 2013

Owner

Shouldn't the "mode" be configurable for each call to select something? I don't see why it's an attribute.

@ericf

ericf Sep 25, 2013

Owner

Shouldn't the "mode" be configurable for each call to select something? I don't see why it's an attribute.

This comment has been minimized.

Show comment Hide comment
@apipkin

apipkin Sep 25, 2013

Contributor

The mode can change based on the use case. You could have a UI element switch the select mode or select mode is different based on a specific criteria. Having it as an attribute allows it to be updated through the api per instance.

@apipkin

apipkin Sep 25, 2013

Contributor

The mode can change based on the use case. You could have a UI element switch the select mode or select mode is different based on a specific criteria. Having it as an attribute allows it to be updated through the api per instance.

This comment has been minimized.

Show comment Hide comment
@ericf

ericf Sep 25, 2013

Owner

I'm saying why isn't this a parameter passed to the select API?

@ericf

ericf Sep 25, 2013

Owner

I'm saying why isn't this a parameter passed to the select API?

This comment has been minimized.

Show comment Hide comment
@apipkin

apipkin Sep 26, 2013

Contributor

The mode isn't for the selectAll method only. The mode is for the DT instance on the whole and is used to set up the delegates when the mode is set.

@apipkin

apipkin Sep 26, 2013

Contributor

The mode isn't for the selectAll method only. The mode is for the DT instance on the whole and is used to set up the delegates when the mode is set.

This comment has been minimized.

Show comment Hide comment
@ericf

ericf Sep 27, 2013

Owner

Yeah I get it, but I'm questioning why you want to maintain state on the DT instance about the mode which selections happen. It seems like the selection mode is something that should be specified each time there's a call to one of the select methods. i.e. the selection mode is related to the act of selecting, not a property of the DT.

e.g. this is analogous and seems wrong:

var person = new Person();

person.set('moveMode', 'left');
person.move();

person.set('moveMode', 'right');
person.move();

Whereas I think this is a better API:

var person = new Person();

person.moveLeft();
person.moveRight();
person.move('left');
@ericf

ericf Sep 27, 2013

Owner

Yeah I get it, but I'm questioning why you want to maintain state on the DT instance about the mode which selections happen. It seems like the selection mode is something that should be specified each time there's a call to one of the select methods. i.e. the selection mode is related to the act of selecting, not a property of the DT.

e.g. this is analogous and seems wrong:

var person = new Person();

person.set('moveMode', 'left');
person.move();

person.set('moveMode', 'right');
person.move();

Whereas I think this is a better API:

var person = new Person();

person.moveLeft();
person.moveRight();
person.move('left');

This comment has been minimized.

Show comment Hide comment
@apipkin

apipkin Sep 27, 2013

Contributor

Yeah I'm not disagreeing with you there at all. But what about when you click on a cell? There has to be a mode set some where to know what to select. I can certainly see adding a param to selectAll that will be used in place of the instance level select mode as well as adding a select('mode') method that will do the same and forward over to ['select' + Mode]() under the hood.

This way programmatically it's succinct and works via UI on the same level

Does that sound reasonable?

@apipkin

apipkin Sep 27, 2013

Contributor

Yeah I'm not disagreeing with you there at all. But what about when you click on a cell? There has to be a mode set some where to know what to select. I can certainly see adding a param to selectAll that will be used in place of the instance level select mode as well as adding a select('mode') method that will do the same and forward over to ['select' + Mode]() under the hood.

This way programmatically it's succinct and works via UI on the same level

Does that sound reasonable?

+
+Y.DataTable.Select = Select;
+
+Y.Base.mix(Y.DataTable, [Y.DataTable.Select]);

This comment has been minimized.

Show comment Hide comment
@ericf

ericf Sep 25, 2013

Owner

EOF newline

@ericf

ericf Sep 25, 2013

Owner

EOF newline

@ericf

This comment has been minimized.

Show comment Hide comment
@ericf

ericf Sep 25, 2013

Owner

@apipkin what's the test coverage for this? And what browsers was it tested in?

Owner

ericf commented Sep 25, 2013

@apipkin what's the test coverage for this? And what browsers was it tested in?

@apipkin

This comment has been minimized.

Show comment Hide comment
@apipkin

apipkin Sep 25, 2013

Contributor

Select.js specifically:

---------------------------------+-----------+-----------+-----------+-----------+
File                             |   % Stmts |% Branches |   % Funcs |   % Lines |
---------------------------------+-----------+-----------+-----------+-----------+
   datatable-select/             |     98.83 |     97.94 |       100 |     98.83 |
      datatable-select.js        |     98.83 |     97.94 |       100 |     98.83 |
---------------------------------+-----------+-----------+-----------+-----------+

I have ran tests in IE6-10, Firefox, Chrome, Safari, and iOS.

[13:58][apipkin:~/Projects/yui3/src/datatable(dt-select)]$ yogi test --coverage
yogi [info]  using yogi@0.1.11 on node@0.10.15
yogi [info]  using module: datatable
yogi [info]  prepping grover tests
yogi [info]  adding tests route
yogi [info]  launching xdr server on port 5001
yogi [info]  listening on: http://127.0.0.1:5000
yogi [info]  turning on coverage support in grover
Starting Grover on 12 files with PhantomJS@1.9.2
  Running 15 concurrent tests at a time.
✔ [DataTable: Message]: Passed: 11 Failed: 0 Total: 11 (ignored 0) (0.131 seconds) 28%
✔ [DataTable: Head]: Passed: 8 Failed: 0 Total: 8 (ignored 0) (0.154 seconds) 98%
✔ [DataTable: Table]: Passed: 34 Failed: 0 Total: 35 (ignored 1) (0.537 seconds) 100%
✔ [DataTable: Base]: Passed: 17 Failed: 0 Total: 17 (ignored 0) (0.353 seconds) 94%
✔ [DataTable: Column Widths]: Passed: 8 Failed: 0 Total: 8 (ignored 0) (0.15 seconds) 86%
✔ [DataTable: select]: Passed: 8 Failed: 0 Total: 8 (ignored 0) (0.206 seconds) 98%
✔ [DataTable: Body]: Passed: 49 Failed: 0 Total: 52 (ignored 3) (0.823 seconds) 87%
✔ [DataTable: Sortable]: Passed: 13 Failed: 0 Total: 13 (ignored 0) (0.224 seconds) 83%
✔ [DataTable: Core]: Passed: 31 Failed: 0 Total: 34 (ignored 3) (0.464 seconds) 93%
✔ [DataTable: Paginator]: Passed: 10 Failed: 0 Total: 10 (ignored 0) (0.405 seconds) 96%
✔ [DataTable: Mutable]: Passed: 98 Failed: 0 Total: 98 (ignored 0) (1.256 seconds) 97%
✔ [Datatable: Scroll]: Passed: 23 Failed: 0 Total: 23 (ignored 0) (1.034 seconds) 77%

---------------------------------+-----------+-----------+-----------+-----------+
File                             |   % Stmts |% Branches |   % Funcs |   % Lines |
---------------------------------+-----------+-----------+-----------+-----------+
   datatable-base/               |     94.44 |     74.29 |       100 |     94.44 |
      datatable-base.js          |     94.44 |     74.29 |       100 |     94.44 |
   datatable-body/               |      87.5 |      72.2 |     93.33 |     87.28 |
      datatable-body.js          |      87.5 |      72.2 |     93.33 |     87.28 |
   datatable-column-widths/      |     86.05 |     71.43 |     88.89 |     86.05 |
      datatable-column-widths.js |     86.05 |     71.43 |     88.89 |     86.05 |
   datatable-core/               |     93.21 |     82.73 |       100 |     93.21 |
      datatable-core.js          |     93.21 |     82.73 |       100 |     93.21 |
   datatable-head/               |     98.98 |     89.29 |       100 |     98.98 |
      datatable-head.js          |     98.98 |     89.29 |       100 |     98.98 |
   datatable-message/            |     28.26 |      12.9 |     30.77 |     28.26 |
      datatable-message.js       |     28.26 |      12.9 |     30.77 |     28.26 |
   datatable-mutable/            |        97 |     88.89 |       100 |        97 |
      datatable-mutable.js       |        97 |     88.89 |       100 |        97 |
   datatable-paginator/          |     96.89 |      88.1 |       100 |     96.89 |
      datatable-paginator.js     |     96.89 |      88.1 |       100 |     96.89 |
   datatable-scroll/             |     77.78 |     60.76 |     80.39 |     77.78 |
      datatable-scroll.js        |     77.78 |     60.76 |     80.39 |     77.78 |
   datatable-select/             |     98.83 |     97.94 |       100 |     98.83 |
      datatable-select.js        |     98.83 |     97.94 |       100 |     98.83 |
   datatable-sort/               |      83.8 |     70.39 |      91.3 |      83.8 |
      datatable-sort.js          |      83.8 |     70.39 |      91.3 |      83.8 |
   datatable-table/              |       100 |     84.09 |       100 |       100 |
      datatable-table.js         |       100 |     84.09 |       100 |       100 |
---------------------------------+-----------+-----------+-----------+-----------+
All files                        |     88.86 |     75.63 |     91.27 |     88.84 |
---------------------------------+-----------+-----------+-----------+-----------+


=============================== Coverage summary ===============================
Statements   : 88.86% ( 1476/1661 )
Branches     : 75.63% ( 953/1260 )
Functions    : 91.27% ( 251/275 )
Lines        : 88.84% ( 1472/1657 )
================================================================================
----------------------------------------------------------------
✔ [Total]: Passed: 310 Failed: 0 Total: 317 (ignored 7) (5.737 seconds)
  [Grover Execution Timer] 5.321 seconds
yogi [info]  grover tests complete
Contributor

apipkin commented Sep 25, 2013

Select.js specifically:

---------------------------------+-----------+-----------+-----------+-----------+
File                             |   % Stmts |% Branches |   % Funcs |   % Lines |
---------------------------------+-----------+-----------+-----------+-----------+
   datatable-select/             |     98.83 |     97.94 |       100 |     98.83 |
      datatable-select.js        |     98.83 |     97.94 |       100 |     98.83 |
---------------------------------+-----------+-----------+-----------+-----------+

I have ran tests in IE6-10, Firefox, Chrome, Safari, and iOS.

[13:58][apipkin:~/Projects/yui3/src/datatable(dt-select)]$ yogi test --coverage
yogi [info]  using yogi@0.1.11 on node@0.10.15
yogi [info]  using module: datatable
yogi [info]  prepping grover tests
yogi [info]  adding tests route
yogi [info]  launching xdr server on port 5001
yogi [info]  listening on: http://127.0.0.1:5000
yogi [info]  turning on coverage support in grover
Starting Grover on 12 files with PhantomJS@1.9.2
  Running 15 concurrent tests at a time.
✔ [DataTable: Message]: Passed: 11 Failed: 0 Total: 11 (ignored 0) (0.131 seconds) 28%
✔ [DataTable: Head]: Passed: 8 Failed: 0 Total: 8 (ignored 0) (0.154 seconds) 98%
✔ [DataTable: Table]: Passed: 34 Failed: 0 Total: 35 (ignored 1) (0.537 seconds) 100%
✔ [DataTable: Base]: Passed: 17 Failed: 0 Total: 17 (ignored 0) (0.353 seconds) 94%
✔ [DataTable: Column Widths]: Passed: 8 Failed: 0 Total: 8 (ignored 0) (0.15 seconds) 86%
✔ [DataTable: select]: Passed: 8 Failed: 0 Total: 8 (ignored 0) (0.206 seconds) 98%
✔ [DataTable: Body]: Passed: 49 Failed: 0 Total: 52 (ignored 3) (0.823 seconds) 87%
✔ [DataTable: Sortable]: Passed: 13 Failed: 0 Total: 13 (ignored 0) (0.224 seconds) 83%
✔ [DataTable: Core]: Passed: 31 Failed: 0 Total: 34 (ignored 3) (0.464 seconds) 93%
✔ [DataTable: Paginator]: Passed: 10 Failed: 0 Total: 10 (ignored 0) (0.405 seconds) 96%
✔ [DataTable: Mutable]: Passed: 98 Failed: 0 Total: 98 (ignored 0) (1.256 seconds) 97%
✔ [Datatable: Scroll]: Passed: 23 Failed: 0 Total: 23 (ignored 0) (1.034 seconds) 77%

---------------------------------+-----------+-----------+-----------+-----------+
File                             |   % Stmts |% Branches |   % Funcs |   % Lines |
---------------------------------+-----------+-----------+-----------+-----------+
   datatable-base/               |     94.44 |     74.29 |       100 |     94.44 |
      datatable-base.js          |     94.44 |     74.29 |       100 |     94.44 |
   datatable-body/               |      87.5 |      72.2 |     93.33 |     87.28 |
      datatable-body.js          |      87.5 |      72.2 |     93.33 |     87.28 |
   datatable-column-widths/      |     86.05 |     71.43 |     88.89 |     86.05 |
      datatable-column-widths.js |     86.05 |     71.43 |     88.89 |     86.05 |
   datatable-core/               |     93.21 |     82.73 |       100 |     93.21 |
      datatable-core.js          |     93.21 |     82.73 |       100 |     93.21 |
   datatable-head/               |     98.98 |     89.29 |       100 |     98.98 |
      datatable-head.js          |     98.98 |     89.29 |       100 |     98.98 |
   datatable-message/            |     28.26 |      12.9 |     30.77 |     28.26 |
      datatable-message.js       |     28.26 |      12.9 |     30.77 |     28.26 |
   datatable-mutable/            |        97 |     88.89 |       100 |        97 |
      datatable-mutable.js       |        97 |     88.89 |       100 |        97 |
   datatable-paginator/          |     96.89 |      88.1 |       100 |     96.89 |
      datatable-paginator.js     |     96.89 |      88.1 |       100 |     96.89 |
   datatable-scroll/             |     77.78 |     60.76 |     80.39 |     77.78 |
      datatable-scroll.js        |     77.78 |     60.76 |     80.39 |     77.78 |
   datatable-select/             |     98.83 |     97.94 |       100 |     98.83 |
      datatable-select.js        |     98.83 |     97.94 |       100 |     98.83 |
   datatable-sort/               |      83.8 |     70.39 |      91.3 |      83.8 |
      datatable-sort.js          |      83.8 |     70.39 |      91.3 |      83.8 |
   datatable-table/              |       100 |     84.09 |       100 |       100 |
      datatable-table.js         |       100 |     84.09 |       100 |       100 |
---------------------------------+-----------+-----------+-----------+-----------+
All files                        |     88.86 |     75.63 |     91.27 |     88.84 |
---------------------------------+-----------+-----------+-----------+-----------+


=============================== Coverage summary ===============================
Statements   : 88.86% ( 1476/1661 )
Branches     : 75.63% ( 953/1260 )
Functions    : 91.27% ( 251/275 )
Lines        : 88.84% ( 1472/1657 )
================================================================================
----------------------------------------------------------------
✔ [Total]: Passed: 310 Failed: 0 Total: 317 (ignored 7) (5.737 seconds)
  [Grover Execution Timer] 5.321 seconds
yogi [info]  grover tests complete
@ericf

This comment has been minimized.

Show comment Hide comment
@ericf

ericf Sep 27, 2013

Owner

I feel that the selection API needs to be revisited, see my comments above.

Owner

ericf commented Sep 27, 2013

I feel that the selection API needs to be revisited, see my comments above.

@apipkin apipkin removed their assignment Mar 15, 2014

@okuryu okuryu assigned apipkin and unassigned apipkin Mar 16, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment