-
-
Notifications
You must be signed in to change notification settings - Fork 131
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
lt-body: add enableScaffolding option #421
lt-body: add enableScaffolding option #421
Conversation
Tests failing... I'll look into that. |
The tests for |
addon/components/cells/base.js
Outdated
classNameBindings: ['align', 'isSorted', 'column.cellClassNames'], | ||
|
||
isSorted: computed.readOnly('column.sorted'), | ||
|
||
style: computed('column.width', function() { |
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.
Are we concerned that this no longer happens when enableScaffolding
is set to false?
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.
I would argue no. Currently, these style
attributes don't really have an effect anyway, as the table-layout: fixed
makes all but the first row ignore custom widths.
As long as the user doesn't overwrite the table-layout: fixed
rule, nothing should change.
The only case where this could matter is, when the user actively decides to change the display
from table
to something like flex
.
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.
I can undo this, or make it depend on whether enableScaffolding
is true
or false
.
addon/components/lt-body.js
Outdated
* @type {Boolean} | ||
* @default true | ||
*/ | ||
enableScaffolding: true, |
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 it would be more sensible to make this a CP that's the inverse of overwrite
.
As we're prepending a scaffolding row, this could lead to some unexpected behavior for people that use selectors like :first-child
.
@offirgolan Would you like to chime in on the issues raised above? I'd like move this PR forward. :) |
@buschtoens this is a risky change as it could introduce many unanticipated issues. Can you give me an example as to how you would create a table with the first row having colspans? Maybe we can tackle this in a different way. |
I needed to implement headings for table sub sections; basically a slimmed down version of the headings in #434 (sticky rows). I tested my branch in our project and couldn't find any problems, but i can understand the hesitation and kinda feel the same. Maybe we could inverse the Alternatively, I'm sure I'll be able to come up with some |
fa85616
to
9ff9d9a
Compare
I made When |
@@ -33,7 +33,7 @@ test('row selection - enable or disable', function(assert) { | |||
|
|||
this.render(hbs `{{lt-body table=table sharedOptions=sharedOptions canSelect=canSelect}}`); | |||
|
|||
let row = this.$('tr:first'); | |||
let row = this.$('tr:not(.lt-scaffolding-row):first'); |
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.
out of curiousity did you need to update these tests? enableScaffolding
isn't set
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.
In the original state of this PR with enableScaffolding
defaulting to true
, this was necessary. It isn't anymore, but I precautiously still included this commit. This way the tests will continue to work, even if we might decide to set enableScaffolding
by default in the future, or when we copy and paste these cases to write some new enableScaffolding
specific tests.
If you feel that this makes the tests harder to comprehend / maintain, I have absolutely no objections to cancel out this commit. 🙂
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.
Got it. That makes sense.
It's a little confusing since it isn't set.
As an aside there aren't any actual tests that verify enableScaffoliding
puts a scaffolding row in the DOM 😲 🚨 🙄
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.
That is... correct. 😨
I could swear, that I've written these. 🤔
Gonna push the tomorrow from the office.
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.
@buschtoens I dont think this is needed since you have specific tests for when it is enabled. I prefer it like that since it is more contained. If we enabled it by default in the future, we can just turn it off for these test cases.
I added a test case asserting, that:
|
scaffoldingRow.hasClass('lt-scaffolding-row'), | ||
'the first row of the <tbody> is a scaffolding row' | ||
); | ||
assert.ok( |
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.
assert.notOk
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.
Actually I was gonna use that, but I wanted to stay in line with the other tests in this file, which all use
assert.ok(!condition);
Should I leave it or still change to notOk
?
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.
lol I didn't even realize.
leave it for now... I'll do a PR to migrate to using ember-native-dom-helpers
in the tests and I'll fix it up there
@offirgolan Any objections? Unless you don't want this feature in ELT, I'll merge. 😉 |
addon/components/cells/base.js
Outdated
let column = this.get('column'); | ||
if (!column) { | ||
return null; | ||
} |
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.
let column = this.get('column');
if (this.get('enableScaffolding') || !column) {
return '';
}
@@ -33,7 +33,7 @@ test('row selection - enable or disable', function(assert) { | |||
|
|||
this.render(hbs `{{lt-body table=table sharedOptions=sharedOptions canSelect=canSelect}}`); | |||
|
|||
let row = this.$('tr:first'); | |||
let row = this.$('tr:not(.lt-scaffolding-row):first'); |
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.
@buschtoens I dont think this is needed since you have specific tests for when it is enabled. I prefer it like that since it is more contained. If we enabled it by default in the future, we can just turn it off for these test cases.
Also removes the style attribute binding from cells/base, if enabled. Also fixes adopted-ember-addons#404.
ef74e49
to
d79382f
Compare
Thanks for reviewing guys. I made the requested changes. Tests are passing as well. |
This PR adds a scaffolding row (just like with
{{lt-head}}
) to the{{lt-body}}
component.This way it is possible to have a first row with colspans.
The scaffolding row can optionally be enabled like so:
Also removes the now unnecessary style attribute binding from
cells/base
, which in theory should yield a small performance boost. 🚀