-
Notifications
You must be signed in to change notification settings - Fork 78
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
Add conversation-part-closed component #1638
Changes from 13 commits
c9115be
bd476e4
7da31f3
e5f797d
600c518
368686b
36220d5
f11705c
8ec511b
955cea9
27f5947
6de8ba1
372f93e
4144f99
9f5c0da
a3f219e
b18b605
f03c50c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,22 @@ | ||
import Component from '@ember/component'; | ||
import { inject as service } from '@ember/service'; | ||
import { computed, get } from '@ember/object'; | ||
import { alias } from '@ember/object/computed'; | ||
|
||
export default Component.extend({ | ||
|
||
author: null, | ||
|
||
classNames: ['conversation-part', 'conversation-part--closed'], | ||
classNameBindings: ['isSelf:conversation-part--is-self'], | ||
|
||
closedAt: null, | ||
|
||
currentUser: service(), | ||
|
||
isSelf: computed('author', 'user', function() { | ||
return get(this, 'author.id') === get(this, 'user.id'); | ||
}), | ||
|
||
user: alias('currentUser.user') | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,13 @@ | ||
<div class="coversation-part__message-wrapper"> | ||
<div class="conversation-part__timestamps-wrapper"> | ||
<div class="conversation-part__timestamps"> | ||
<span data-test-closed-at> | ||
{{#if isSelf}} | ||
You closed this {{moment-from-now closedAt interval=60000}} | ||
{{else}} | ||
{{author.username}} closed this {{moment-from-now closedAt interval=60000}} | ||
{{/if}} | ||
</span> | ||
</div> | ||
</div> | ||
</div> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In line with my comment above, you can remove all the |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,7 +1,7 @@ | ||
<div class="conversation-thread-inner"> | ||
<div class="conversation-thread__messages"> | ||
{{#if conversation.message.isLoaded}} | ||
{{conversations/conversation-part | ||
{{conversations/conversation-part-comment | ||
author=conversation.message.author | ||
body=conversation.message.body | ||
readAt=conversation.message.readAt | ||
|
@@ -10,16 +10,23 @@ | |
{{/if}} | ||
{{#each conversation.conversationParts as |conversationPart|}} | ||
{{#if conversationPart.isLoaded}} | ||
{{conversations/conversation-part | ||
author=conversationPart.author | ||
body=conversationPart.body | ||
readAt=conversationPart.readAt | ||
sentAt=conversationPart.insertedAt | ||
}} | ||
{{#if (eq conversationPart.partType 'isClosed')}} | ||
{{conversation/conversation-part-closed | ||
author=conversationPart.author | ||
closedAt=conversationPart.insertedAt | ||
}} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Indentation here is off. |
||
{{else}} | ||
{{conversations/conversation-part-comment | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Your indentation here is wrong. |
||
author=conversationPart.author | ||
body=conversationPart.body | ||
readAt=conversationPart.readAt | ||
sentAt=conversationPart.insertedAt | ||
}} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Indentation here is also off. |
||
{{/if}} | ||
{{/if}} | ||
{{/each}} | ||
</div> | ||
<div class="conversation-thread__composer"> | ||
<div class="conversation-thread__composer"> | ||
{{conversations/conversation-composer | ||
send=(pipe (action send) (action scrollBottom)) | ||
}} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,73 @@ | ||
import { moduleForComponent, test } from 'ember-qunit'; | ||
import hbs from 'htmlbars-inline-precompile'; | ||
import { set } from '@ember/object'; | ||
import PageObject from 'ember-cli-page-object'; | ||
import component from 'code-corps-ember/tests/pages/components/conversations/conversation-part'; | ||
import stubService from 'code-corps-ember/tests/helpers/stub-service'; | ||
import moment from 'moment'; | ||
|
||
let page = PageObject.create(component); | ||
|
||
function renderPage() { | ||
page.render(hbs` | ||
{{conversations/conversation-part-closed | ||
author=author | ||
closedAt=closedAt | ||
}} | ||
`); | ||
} | ||
|
||
moduleForComponent('conversations/conversation-part-closed', 'Integration | Component | conversations/conversation part closed', { | ||
integration: true, | ||
beforeEach() { | ||
page.setContext(this); | ||
}, | ||
afterEach() { | ||
page.removeContext(); | ||
} | ||
}); | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would love feedback on test statements on 21 + 25. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For future reference, you should comment below those lines so we can see them in this view. Also, would be helpful to have some direction in terms of what you want feedback on specifically? |
||
test('if current user closes message, "You closed this" is rendered', function(assert) { | ||
assert.expect(1); | ||
|
||
let user = { | ||
id: 1, | ||
username: 'testuser' | ||
}; | ||
set(this, 'author', user); | ||
stubService(this, 'current-user', { user }); | ||
|
||
let twoMinutesAgo = moment().subtract(2, 'minutes'); | ||
let twoMinutesAgoFriendly = twoMinutesAgo.from(); | ||
set(this, 'closedAt', twoMinutesAgo); | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For the life of me, I can't figure out how to get the timestamp to pass in and have been working on it for an very inefficient length of time, by that I mean all day. This thing has kicked my ass to and fro and I'm sick of looking at it and am going to move to something else so the day isn't a complete wash. Nikola has stepped in a bunch with some guidance, but it didn't seem to help my stupidity. |
||
renderPage(); | ||
|
||
let expectedText = `You closed this ${twoMinutesAgoFriendly}`; | ||
assert.equal(page.closedAt.text, expectedText, 'The closed at timestamp is rendered'); | ||
}); | ||
|
||
test('if someone other than the current user closes the message, "Author.username closed this at" is rendered', function(assert) { | ||
assert.expect(1); | ||
|
||
let user = { | ||
id: 1, | ||
username: 'currentuser' | ||
}; | ||
stubService(this, 'current-user', { user }); | ||
|
||
let author = { | ||
id: 2, | ||
username: 'authoruser' | ||
}; | ||
set(this, 'author', author); | ||
|
||
let twoMinutesAgo = moment().subtract(2, 'minutes'); | ||
let twoMinutesAgoFriendly = twoMinutesAgo.from(); | ||
set(this, 'closedAt', twoMinutesAgo); | ||
|
||
renderPage(); | ||
|
||
let expectedText = `${author.username} closed this ${twoMinutesAgoFriendly}`; | ||
assert.equal(page.closedAt.text, expectedText, 'The closed at timestamp is rendered'); | ||
}); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This one is good, but I would call it The other test is |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,6 +11,14 @@ export default { | |
scope: '[data-test-body]' | ||
}, | ||
|
||
closedAt: { | ||
scope: '[data-test-closed-at]' | ||
}, | ||
|
||
isComment: hasClass('conversation-part--comment'), | ||
isClosed: hasClass('conversation-part--closed'), | ||
isByCurrentUser: hasClass('conversation-part--is-self'), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I saw a question that got marked as outdated, which caused me to think page objects might not be completely clear to you. They are only ever used in tests and they are defined in a way that provides us with information we need to answer questions in tests. If a conversation thread lists multiple conversations of different
We know that a comment has a assert.ok($('conversation-part').classNames().indexOf('conversation-part--comment') > -1);
assert.ok($('conversation-part').classNames().indexOf('conversation-part--closed') > -1); and that works, except it's not always completely clear what it is we're actually asking. Also, in multiple tests, we tend to ask the same or similar questions, so we have to repeat all of that. Then, if, for example, the class name changes, we have to change it in all of those places. What page objects allow us to do is to specify isClosed: hasClass('conversation-part--closed'), in just one place and use that. assert.ok(conversationPart.isClosed);
assert.ok(conversationPart.isComment); That makes the question much clearer. Also, if the class changes, we change it in just that one place - in the page object. |
||
|
||
photo: { | ||
scope: '[data-test-target-photo]', | ||
url: attribute('src') | ||
|
@@ -23,7 +31,5 @@ export default { | |
|
||
sentAt: { | ||
scope: '[data-test-sent-at]' | ||
}, | ||
|
||
sentByCurrentUser: hasClass('conversation-part--is-self') | ||
} | ||
}; |
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.
You have indentation issues here.