Add conversation status selector #1647
Conversation
joshsmith
force-pushed
the
1632-add-conversation-status-select
branch
from
ae3dcb0 to
2d031b3
Compare
app/routes/project/conversations.js
Outdated
| let controller = this.controllerFor('project.conversations'); | ||
| controller.set('currentlyLoading', true); | ||
| let that = this; | ||
| transition.promise.finally(function() { |
There was a problem hiding this comment.
can a hash rocket be used here?
|
|
||
| set(this, 'status', 'open'); | ||
|
|
||
| renderPage(); |
There was a problem hiding this comment.
does renderPage need an away? I'm guessing not since this passes, but have seen await with render in the new qunit api
There was a problem hiding this comment.
It might need it of we're setting a component property without wrapping it in a run. Someone ought to try that out.
| @@ -214,11 +212,28 @@ test('Project admin can reopen a conversation', function(assert) { | |||
| project: project.slug | |||
| }); | |||
|
|
|||
| andThen(() => { | |||
| page.statusSelect.openButton.click(); | |||
There was a problem hiding this comment.
The new async await would be rid of the andThens'. I'm sure you already know that but I found it was useful to start moving acceptance tests over 1 test file at a time (maybe a codemod) available now???
| queryParams: ['status'], | ||
| status: 'open', | ||
|
|
||
| currentlyLoading: null, |
There was a problem hiding this comment.
do you want the default to be false?
There was a problem hiding this comment.
I don't think so, since it could result in some weird state.
app/routes/project/conversations.js
Outdated
| transition.promise.finally(function() { | ||
| controller.set('currentlyLoading', false); | ||
| that.transitionTo('project.conversations.index'); | ||
| }); |
There was a problem hiding this comment.
using an arrow function should eliminate the need for that
| if (get(conversations, 'length') > 0) { | ||
| this.transitionTo('project.conversations.conversation', get(conversations, 'firstObject')); | ||
| let sortedConversations | ||
| = conversations.filterBy('status', status).sortBy('updatedAt'); |
There was a problem hiding this comment.
I don't think you need to filter here, right? The model should already be filtered. If it's not, then we should add a comment as to why that is.
You do need to sort, but not until you know the array isn't empty. That shouldn't matter much, though. Sorting a blank array is not expensive.
There was a problem hiding this comment.
The filtering does need to happen since the model does not necessarily update when an individual conversation changes.
|
|
||
| set(this, 'status', 'open'); | ||
|
|
||
| renderPage(); |
There was a problem hiding this comment.
It might need it of we're setting a component property without wrapping it in a run. Someone ought to try that out.
joshsmith
force-pushed
the
1632-add-conversation-status-select
branch
from
f4e08f1 to
8bdce3c
Compare
joshsmith
force-pushed
the
1632-add-conversation-status-select
branch
from
8bdce3c to
2c3455f
Compare
| andThen(() => { | ||
| page.conversations(1).click(); | ||
| }); | ||
| page.conversations(1).click(); |
There was a problem hiding this comment.
await here as well? I thik it may b/c click is an async test helper.
| andThen(() => { | ||
| page.conversations(0).click(); | ||
| }); | ||
| page.conversations(0).click(); |
What's in this PR?
Add a selector to change the conversation status.
Needs tests.
References
Progress on: #1632