-
Notifications
You must be signed in to change notification settings - Fork 355
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[restructure] New Table API #517
Conversation
cell=cell | ||
}} | ||
{{#ember-table as |t|}} | ||
{{ember-thead api=t columns=columns}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe table=t
instead of api=t
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I've been going back and forth on that one. On the one hand, it reads better for now, but on the other I think it may actually obfuscate what we're passing in and why. This especially becomes an issue later on:
{{#ember-tbody as |b|}}
{{#ember-tr body=b as |r|}}
{{#ember-td row=r as |value cell column row|}}
^r and row are not the same thing
{{/ember-td}}
{{/ember-tr}}
{{/ember-tbody}}
I think it may make sense to add the name as an alias to each component, so users can choose whichever one makes more sense to them and we can see which is less confusing over time. Once we get to 2.3 we'll be converting to contextual components anyways, so it's not really a big deal either way.
d9dea3b
to
f55407e
Compare
@argument | ||
@type('string') | ||
@attribute('aria-label') | ||
ariaLabel; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this ariaLabel used for ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's for the a11y test, checkboxes are now enabled based on selectMode
so they're enabled for all tests. a11y test checks to make sure all form controls are properly labeled.
cells=cells) | ||
}} | ||
{{else}} | ||
aoeu |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo?
LGTM barring some comments |
Refactors to the the broken out table API outlined in #505. Notes: * Lots of APIs renamed and moved * `thead` * `columns` * `numFixedColumns` = `hasFixedColumn` (also now a boolean) * `onColumnResize` => `onResize` * `onColumnReordered` => `onReorder` * `columnMode` => `resizeMode` * `tableResizeMode` => `fillMode` * `tbody` * `tree` * `rows` * `estimateRowHeight` * `onSelect` * `selectedRows` * `selectionMode` => `selectMode` * `tfoot` * `tree` * `rows` * `estimateRowHeight` * `onSelect` * `selectedRows` * `selectionMode` => `selectMode` * Checkboxes now render IFF `onSelect` exists AND `selectMode === 'multiple'`, which is a much easier heuristic to follow and enforces that the checkbox is always in the first cell * Had to restructure the RowWrapper a bit to avoid backtracking rerender * Headers/columns need to be reworked quite a bit, will be doing this in a followup PR
f55407e
to
c756e41
Compare
Refactors to the the broken out table API outlined in #505. Notes:
thead
columns
numFixedColumns
=hasFixedColumn
(also now a boolean)onColumnResize
=>onResize
onColumnReordered
=>onReorder
columnMode
=>resizeMode
tableResizeMode
=>fillMode
tbody
tree
rows
estimateRowHeight
onSelect
selectedRows
selectionMode
=>selectMode
tfoot
tree
rows
estimateRowHeight
onSelect
selectedRows
selectionMode
=>selectMode
onSelect
exists ANDselectMode === 'multiple'
, which is a much easier heuristic to follow and enforcesthat the checkbox is always in the first cell
a followup PR