Skip to content
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.

Sign up for GitHub

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

Merged
merged 18 commits into from
Jan 16, 2018
Merged

Add conversation-part-closed component #1638

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
c9115be
added conversation-part-closed component
daveconnis Dec 20, 2017
bd476e4
changed component name from conversation-part to conversation-part-co…
daveconnis Dec 21, 2017
7da31f3
fix component-part naming
daveconnis Dec 21, 2017
e5f797d
write page object and test for component
daveconnis Jan 1, 2018
600c518
start tests for conversation-part-closed
daveconnis Jan 2, 2018
368686b
work on getting tests to pass
daveconnis Jan 4, 2018
36220d5
write tests for conversation-part-closed
daveconnis Jan 4, 2018
f11705c
Fix integration tests
daveconnis Jan 8, 2018
8ec511b
change 'text' to 'expectedText'
daveconnis Jan 9, 2018
955cea9
make changes to dry up code
daveconnis Jan 10, 2018
27f5947
change conversation-part/conversation-part-comment with tests passing
daveconnis Jan 10, 2018
6de8ba1
fix syntax and alpha-ordering
daveconnis Jan 10, 2018
372f93e
fix syntax and remove unnecessary code
daveconnis Jan 11, 2018
4144f99
fix indentation
daveconnis Jan 11, 2018
9f5c0da
Merge branch 'develop' into conversation-part-for-open-and-closed-sta…
begedin Jan 15, 2018
a3f219e
Update conversation-thread.hbs
begedin Jan 15, 2018
b18b605
reordered conversation-part-closed to match style guide
daveconnis Jan 15, 2018
f03c50c
Update conversation-part-closed.js
begedin Jan 16, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 0 additions & 3 deletions app/components/conversations/conversation-part-closed.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,5 @@ export default Component.extend({

isSelf: computed('author', 'user', function() {
return get(this, 'author.id') === get(this, 'user.id');


})
})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two problems here

It's not clear what author is

You specify the value of author through the .hbs, as exampled in the test module you wrote:

{{conversations/conversation-part-closed author=author closedAt=closedAt}}

When we do that, we also add that property into the .js and give it a default value, usually null, unless some other value makes sense. You already did this with closedAt: null, so you should do the same with author: null.

The code will work regardless, but we do this so that, when someone is reading the .js, they do not need to navigate back and forth between the .js and the .hbs and the component is relatively clear from the .hbs alone.

user is not defined at all

The author is whichever user is responsible for this specific conversation part being created.

The user is supposed to be whichever user is currently signed in to the app. You get it from the currentUser service as currentUser.user, so to have it available to you as just user, you need to define an alias. You can look at the basic conversation-part-comment component to see how that's done.

});
12 changes: 8 additions & 4 deletions app/templates/components/conversations/conversation-part-closed.hbs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,13 @@
<div class="coversation-part-closed__message-wrapper">
{{#if currentUser}}
You closed this at... You closed this at {{moment-format conversationPart.updatedAt 'MM/DD/YYYY hh:mm:ss'}}
<div class="conversation-part-closed__timestamps-wrapper">
<div class="conversation-part-closed__timestamps">
<span data-test-closed-at>
{{#if isSelf}}
You closed this at {{moment-format conversationPart.updatedAt 'MM/DD/YYYY hh:mm:ss'}}
{{else}}
<div class="conversation-part-closed__message-wrapper">
{{author.username}} closed this at {{moment-format conversationPart.updatedAt 'MM/DD/YYYY hh:mm:ss'}}
{{/if}}
{{/if}}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indentation error here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still have this indentation error, It starts at line 5, if it's not clear. The whole if-else chunk needs to be indented 3 levels deeper.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You still have an indentation error here.

</span>
</div>
</div>
</div>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In line with my comment above, you can remove all the -closed parts from all the classes here.

21 changes: 21 additions & 0 deletions tests/integration/components/conversations/conversation-part-closed-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,31 @@
import { moduleForComponent, test } from 'ember-qunit';
import hbs from 'htmlbars-inline-precompile';
import PageObject from 'ember-cli-page-object';
import component from 'code-corps-ember/tests/pages/components/conversations/conversation-part-closed';
import stubService from 'code-corps-ember/tests/helpers/stub-service';

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
});

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would love feedback on test statements on 21 + 25.

Copy link
Contributor

Choose a reason for hiding this comment

The 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 user closes message, "You closed this at" is rendered', function(assert){

})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These blank tests seems like they would test correct behavior, so the thinking is probably sound. Just need to implement them.


test('if user looks at closed message they did not close, "author.username" is rendered', function(assert){

})

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

test('it renders', function(assert) {

// Set any properties with this.set('myProperty', 'value');
Expand Down
12 changes: 10 additions & 2 deletions tests/pages/components/conversations/conversation-part-closed.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,15 @@
import {
text
hasClass
} from 'ember-cli-page-object';

export default {
title: text('h1')
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would also love feed back on page object. Think I have everything I need though, at least that I've found I need so far.


scope: '.conversation-part-closed',

closedAt: {
scope: '[data-test-closed-at]'
},

sentByCurrentUser: hasClass('conversation-part-closed--is-self')

};