-
Notifications
You must be signed in to change notification settings - Fork 286
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
Tidy up render-tree tab #1025
Tidy up render-tree tab #1025
Conversation
app/controllers/render-tree.js
Outdated
} | ||
|
||
return this.model.filter((item) => { | ||
const regExp = new RegExp(this.escapedSearch); | ||
return !!recursiveMatch(item, regExp); |
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'm not sure what the !!
is here for since recursiveMatch
seems to only return a boolean value. Am I crazy?
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.
Ya, its not needed.
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.
@nummi if it's not needed, should we go ahead and remove?
9b79c27
to
9a9f463
Compare
9a9f463
to
d3e094a
Compare
@@ -58,6 +58,12 @@ | |||
} | |||
} | |||
|
|||
.list.list--css-striping { |
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.
This just seemed like the easiest way to get striping working again.
🤷♂️
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.
Is there not an existing class for striping?
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.
We do. It’s based on passing a boolean to a row. I think I was struggling with this since the dataset is nested.
app/controllers/render-tree.js
Outdated
} | ||
|
||
return this.model.filter((item) => { | ||
const regExp = new RegExp(this.escapedSearch); | ||
return !!recursiveMatch(item, regExp); |
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.
Ya, its not needed.
d3e094a
to
c596af0
Compare
@@ -114,24 +114,6 @@ module('Render Tree Tab', function(hooks) { | |||
assert.dom(rows[0].querySelector('.js-render-profile-name')).hasText('First View Rendering'); | |||
assert.dom(rows[1].querySelector('.js-render-profile-name')).hasText('Second View Rendering'); | |||
|
|||
await fillIn('.js-render-profiles-search input', '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.
@nummi do we not need these tests anymore?
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.
Search filtering is still tested in the lines below these — the code removed here was testing the (broken, see GIF) functionality of opening of children rows.
68a011f
to
a584904
Compare
- fix table striping - remove observer - fix search highlighting - action, this, {{on}}, etc. - use Ui:Disclosure
a584904
to
a4fbd1d
Compare
I simply removed the observer for now because the functionality it provided is not very helpful:
I believe this was meant to expand every list item. In the future we could expand the tree where necessary.