feat(checkbox): Adds a checkbox option to columns by pzuraq · Pull Request #484 · Addepar/ember-table

pzuraq · 2018-02-02T05:58:12Z

Adds a checkbox option to table columns which can be used to toggle the row.
Also refactors row selection to be DDAU, and adds tests and creates a simpler
generateTable function

billy-addepar · 2018-02-02T17:45:14Z

+  @attribute
+  type = 'checkbox';
+
+  @argument({ defaultIfUndefined: true })


Should its value defaulted to be false?

Yeah, I think so

billy-addepar · 2018-02-02T17:59:08Z

  }

+  selectRow = (rowIndex, { toggle, range }) => {
+    let rows = this.get('rows');


You might want handle SELECTION_MODE_NONE here

check tests, SELECTION_MODE_NONE is simply not adding an action for onSelect

billy-addepar · 2018-02-02T18:07:19Z

+
+const alpha = 'ABCDEFGHIJKLMNOPQRSTUVWXYZ';
+
+function hex(a) {


This function looks complicated. If I am not wrong, you are trying to convert a number to Excel style alphabet index. Can you use simpler function in tests?

I copied this from online somewhere, I’m sure there’s a lib somewhere. The point is it’s converting us to base26 instead of just modulo, so each cell is verifiably unique.

Can you use simpler method here? Like using "Cell" + the input number itself if you want to identify each cell.

This function takes me a while to figure what it does. If there is a bug, it's hard to debug. This is for testing purpose and we do not really need complicated function.

we need to identify each cell by row and column, which is a change I made to the testing template as well. You can't just check that a cell has content A, it need to be 1A or 2A or 3A or 1AA, so on.

Alternatively, we can try to have some longer content such as Row1Column1 to check against, this just seemed simpler (I just googled base26 and copied stack overflow). We can also try to see if there's a Node library that does this, it should be very standard.

https://www.npmjs.com/package/base26

Looks like there isn't an AMD build yet so unfortunately we can't use it 😕 I really don't think it's that big a deal, it's a simple function unlikely to have bugs.

Cool to me. I would not block the PR because of this.

billy-addepar · 2018-02-02T18:08:19Z

+  return s;
+}
+
+const fullTable = hbs`


There are many duplication of this helper file and the ones in test-scenario. Can you merge them?

That’s the plan! The APIs are different so I didn’t want to block this PR on them, the next one will be cleanup and expand tests in general, then Tree model refactors

Sounds good to me

billy-addepar · 2018-02-02T18:15:32Z

👍 General approach looks good. Just need some clean up

billy-addepar · 2018-02-02T19:15:55Z

I tried out this branch and got an error:

"Error: Assertion Failed: Attempted to assign 'isSelected' value on a (unknown mixin) component, but no argument was defined for that key. Use the @argument helper on the class field to define an argument which can be passed into the component
 at new EmberError (http://localhost:4200/assets/vendor.js:42116:25)↵    
at Object.assert (http://localhost:4200/assets/vendor.js:42359:15)↵    
at Function.create (http://localhost:4200/assets/vendor.js:87979:49)↵   
 at Function.superWrapper [as create] (http://localhost:4200/assets/vendor.js:74175:22)↵    at FactoryManager.create (http://localhost:4200/assets/vendor.js:38866:25)↵    
at Proxy.create (http://localhost:4200/assets/vendor.js:38610:20)↵   
 at CurlyComponentManager.create (http://localhost:4200/assets/vendor.js:43607:31)↵    
at http://localhost:4200/assets/vendor.js:31053:33↵    
at AppendOpcodes.evaluate (http://localhost:4200/assets/vendor.js:29452:13)↵    
at VM.next (http://localhost:4200/assets/vendor.js:36628:32)"

Adds a checkbox option to table columns which can be used to toggle the row. Also refactors row selection to be DDAU, and adds tests and creates a simpler generateTable function

billy-addepar reviewed Feb 2, 2018

View reviewed changes

pzuraq force-pushed the pzuraq/add-checkbox-to-tables branch 2 times, most recently from 2e019d9 to b256ec2 Compare February 6, 2018 02:19

feat(checkbox): Adds a checkbox option to columns

ba11b76

Adds a checkbox option to table columns which can be used to toggle the row. Also refactors row selection to be DDAU, and adds tests and creates a simpler generateTable function

pzuraq force-pushed the pzuraq/add-checkbox-to-tables branch from b256ec2 to ba11b76 Compare February 6, 2018 19:25

billy-addepar approved these changes Feb 6, 2018

View reviewed changes

pzuraq merged commit 0c562f9 into master Feb 6, 2018

pzuraq deleted the pzuraq/add-checkbox-to-tables branch February 6, 2018 21:19


		const alpha = 'ABCDEFGHIJKLMNOPQRSTUVWXYZ';

		function hex(a) {

Conversation

pzuraq commented Feb 2, 2018

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!

billy-addepar commented Feb 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

billy-addepar commented Feb 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

2 participants

billy-addepar commented Feb 2, 2018 •

edited

Loading

billy-addepar commented Feb 2, 2018 •

edited

Loading