create volunteer-headshot component by WenInCode · Pull Request #788 · code-corps/code-corps-ember

WenInCode · 2016-11-27T19:17:49Z

What's in this PR?

Changes

Created volunteer-headshot component
Created volunteer-headshot page-object
Added tests

Todo

add styling
use the component in the thankyou page.

References

Fixes #678

joshsmith · 2016-11-27T20:07:07Z

@WenInCode think maybe there's a misunderstanding here. We want to take one of the roles out of the user's own user-roles and use that for display.

WenInCode · 2016-11-27T21:09:26Z

@joshsmith ha that makes so much more sense

joshsmith · 2016-11-27T21:10:02Z

@WenInCode sorry! Did I not explain it well enough in the issue?

joshsmith · 2016-11-27T21:10:22Z

I think I didn't. Was pretty vague.

joshsmith · 2016-11-30T05:58:06Z

@WenInCode is this reviewable now?

WenInCode · 2016-11-30T05:59:56Z

@joshsmith negative, working on putting it on the thank-you page and need to add styling. Working on it right now, but I will likely finish tomorrow as it's pretty late.

WenInCode · 2016-12-01T05:13:27Z

I didn't get to work as much as I had hoped to over the past two days, but here is some progress (I only had 1 contributor so I hard coded more). There are still some things that need to be finished.

styles
selecting subset of contributors

It has been fun to try and do some scss even if I am not great at it yet. Sorry this is taking longer than expected.

joshsmith · 2016-12-02T07:03:58Z

@WenInCode this is great progress. Also, you look like a pretty prolific person there! Backend Developer and Tester and Data Scientist. 🔥 🔥 🔥 🚒

WenInCode · 2016-12-04T17:32:53Z

Updated a bunch of things. The volunteer list loads a bit slowly because the userRoles or roles (forgot which) aren't side loaded.

One thing I have not figured out is how to center-align the list of contributors. I could use some help there. Otherwise, I think this is ready for review.

Tests are passing locally, not sure why they were killed in circle.

Here's an image of 12 of me as bill murray:

marineb · 2016-12-04T20:53:31Z

@WenInCode I'd be happy to help center align the contributors (though I'm not sure how to checkout your branch). Let me know if you still want someone to chime in.

sbatson5

Just a few suggestions/questions. This is looking great 👍

sbatson5 · 2016-12-04T20:53:40Z

+        return get(orgMembership, 'member');
+      });
+
+      if (volunteers.length > 12) {


Thoughts on extracting 12 out to a constant at the top of this component? It seems like a magic number when reading over this. if (volunteers.length > MAX_VOLUNTEERS) may make it a bit more clear.

Ya that sounds good to me. Normally when I'm working with magic numbers I would remember to do this! Good catch 👍

sbatson5 · 2016-12-04T21:00:19Z

+    @property volunteerName
+    @type String
+   */
+  volunteerName: computed('volunteer.name', 'volunteer.firstName', 'volunteer.lastName', function() {


If we are observing multiple properties on 1 object, we can simplify what we are observing to:
computed('volunteer.{name,firstName,lastName}', ...

sbatson5 · 2016-12-04T21:07:38Z

+    <p class="volunteer-headshot__role" data-test-selector="volunteer role">{{userRole.role.name}}</p>
+  {{else}}
+    <p class="volunteer-headshot__role" data-test-selector="volunteer role">Volunteer</p>
+  {{/if}}


Rather than doing the if/else block, it looks like we can get away with an inline if since we are only updating 1 item.

<p class="volunteer-headshot__role" data-test-selector="volunteer role"> {{if userRole userRole.role.name "Volunteer"}} </p>

I completely forgot about the inline if 😜 - thanks!

sbatson5 · 2016-12-04T21:13:57Z

+  'Designer & Developer'
+];
+
+function generateUserRoles() {


Since we are just adding 1 object per role to this array (i.e. the number of roles never varies), thoughts on just making it static to start with:

const usersRoles = [ { role: { name: 'Developer' } }, { role: { name: 'Ember Developer' } }, { role: { name: 'UX Designer' } }, { role: { name: 'Software Engineer' } }, { role: { name: 'Project Coordinator' } }, { role: { name: 'Designer & Developer } }' ];

This way we aren't rebuilding this on every test.

WenInCode · 2016-12-05T00:38:32Z

Thanks @sbatson5 for the feedback!

@marineb help would be greatly appreciated! The way I work with someone else's fork is the following:

add the remote

git remote add [whatever you want to call the remote] [the remotes url]

# ex.
git remote add wenincode https://github.com/wenincode/code-corps-ember

fetch the changes

git fetch wenincode 

# or get all
git fetch --all

checkout branches

and then you chould be able to checkout into the branch.

git checkout add-volunteer-headshot-component

push changes

When your ready just push to your changes to my remote (I will give you commit access to my repo)

git push wenincode add-volunteer-headshot-component

I have never actually pushed changes to someones remote, but this is the flow I normally use if I want to pull down someones changes.

WenInCode · 2016-12-05T01:11:14Z

@sbatson5 I've addressed the feedback you've given - thanks again!
Putting this back into review.

marineb · 2016-12-05T04:13:04Z

@WenInCode I wasn't able to push directly to your codebase, so I sent you a PR.

joshsmith · 2016-12-06T03:43:50Z

+    @property nonPendingMemberships
+    @type Ember.Array
+   */
+  nonPendingMemberships: computed('organizationMemberships.[]', 'organizationMemberships.@each.role', function() {


What is the reason for doing .[] and .@each.role?

@joshsmith I had an idea of why I was doing this, but I wasn't sure I was right. So I threw together this twiddle to show the differences.

computed property on an array of objects won't recompute on things being pushed/popped from the array if obj.[] is not used.

computed property on an array of objects won't recompute if an object in that arrays attributes change without @each.thatattribute

However, I agree with your statement below, so this will probably be taken out. But I figured I should still investigate.

@WenInCode I figured this was the case and I reviewed top to bottom here, so the other comments are laggard comments.

joshsmith · 2016-12-06T03:44:50Z

+
+    if (isPresent(memberships)) {
+      return memberships.filter((membership) => {
+        let isPending = get(membership, 'isPending');


The organization model has a line pendingMemberships: filterBy('organizationMemberships', 'isPending'). Maybe we could have an approvedMemberships and then you can just compute on the approvedMemberships?

agreed. one thing is that the role has to be fetched, so there is a portion of time where role on the memberships is undefined. During that loading, all memberships with undefined roles will be not pending.

I guess what I am getting at is the cleanest way to do it is (and the way I have changed it to):
approvedMemberships: computed.filterBy('organizationMemberships', 'isNotPending'),
but that does not filter properly when role is undefined.

joshsmith · 2016-12-06T03:46:28Z

+    if (isPresent(nonPendingMemberships)) {
+      let volunteers = nonPendingMemberships.map((orgMembership) => {
+        return get(orgMembership, 'member');
+      });


You can do Ember.computed.mapBy instead. See above for how to put an approvedMembers directly on the organization model.

joshsmith · 2016-12-06T03:47:00Z

+  volunteers: computed('nonPendingMemberships', function() {
+    let nonPendingMemberships = get(this, 'nonPendingMemberships');
+
+    if (isPresent(nonPendingMemberships)) {


Do you need isPresent here?

joshsmith · 2016-12-06T03:47:34Z

+      'volunteer.userRoles.@each.user',
+    function() {
+      return this.get('volunteer.userRoles');
+    }),


Why not just an alias here?

joshsmith · 2016-12-06T03:48:52Z

+    } else if (isPresent(firstName) && isPresent(lastName)) {
+      return `${firstName} ${lastName}`;
+    } else {
+      return '';


In the else would fallback to their username, which we know must be set.

joshsmith · 2016-12-06T03:49:30Z

+    @property isVolunteerNamePresent
+    @type Boolean
+   */
+  isVolunteerNamePresent: computed.notEmpty('volunteerName'),


Can remove this if you implement the above comment.

ya I had originally put it because I was getting some fun undefined undefined as names, because the members promise hadn't resolved.

joshsmith · 2016-12-06T03:50:00Z

+  altText: computed('volunteerName', function() {
+    let name = get(this, 'volunteerName');
+
+    return `${name}\'s headshot`;


Would remove "headshot" here, and let the alt text just be their name.

joshsmith · 2016-12-06T03:52:10Z

+  beforeEach() {
+    this.set('user', {
+      name: 'Test User',
+      photoThumbUrl: 'http://fillmurray.com/200/200',


Can we use a URL here that doesn't make an extra HTTP request?

@joshsmith that's a good idea... but no more great photos of Bill Murray in tests.

WenInCode · 2016-12-07T05:03:48Z

I believe I have addressed all the feedback. I rebased as there was a conflict with develop.

This should be ready for review. Thanks to all who have put time into this!

joshsmith · 2016-12-08T07:03:25Z

@WenInCode can you repush to restart the build? Thanks!

joshsmith · 2017-01-26T19:35:00Z

@WenInCode to fix the build, try to change circle.yml L30 from ./node_modules/coveralls/bin/coveralls.js < ./coverage/lcov.info to COVERAGE=true ./node_modules/coveralls/bin/coveralls.js < ./coverage/lcov.info

joshsmith · 2017-02-10T07:52:40Z

@WenInCode there are some conflicts now due to some upstream CSS changes merging in #846.

WenInCode · 2017-03-01T16:37:38Z

@joshsmith should be good to review again

WenInCode added in progress Priority: Medium labels Nov 27, 2016

joshsmith modified the milestones: Donations, Improve Donations Nov 27, 2016

WenInCode force-pushed the add-volunteer-headshot-component branch from 14f26dc to d6a7dee Compare December 1, 2016 05:11

WenInCode added needs code review and removed in progress labels Dec 4, 2016

sbatson5 reviewed Dec 4, 2016

View reviewed changes

WenInCode added needs submitter response and removed needs code review labels Dec 5, 2016

WenInCode added needs code review and removed needs submitter response labels Dec 5, 2016

joshsmith reviewed Dec 6, 2016

View reviewed changes

joshsmith added the needs submitter response label Dec 6, 2016

joshsmith removed the needs code review label Dec 6, 2016

joshsmith reviewed Dec 6, 2016

View reviewed changes

WenInCode force-pushed the add-volunteer-headshot-component branch 2 times, most recently from 12baefb to d0816eb Compare December 7, 2016 05:02

WenInCode added needs code review and removed needs submitter response labels Dec 7, 2016

WenInCode force-pushed the add-volunteer-headshot-component branch from d0816eb to d522986 Compare December 8, 2016 15:37

WenInCode force-pushed the add-volunteer-headshot-component branch from b508ac6 to 0580a9a Compare January 26, 2017 00:40

WenInCode force-pushed the add-volunteer-headshot-component branch from 29b9c58 to e7e8f49 Compare February 1, 2017 17:45

WenInCode force-pushed the add-volunteer-headshot-component branch from e7e8f49 to 2972d36 Compare March 1, 2017 16:37

joshsmith force-pushed the add-volunteer-headshot-component branch 3 times, most recently from 74c3d35 to 5f562f2 Compare March 29, 2017 20:59

Create volunteer-headshot component

dcbb7e9

joshsmith force-pushed the add-volunteer-headshot-component branch from 5f562f2 to dcbb7e9 Compare March 29, 2017 21:00

joshsmith removed the Priority: Medium label Mar 29, 2017

joshsmith approved these changes Mar 29, 2017

View reviewed changes

joshsmith added ready to merge and removed needs code review labels Mar 29, 2017

joshsmith merged commit 5488f92 into code-corps:develop Mar 29, 2017

joshsmith deleted the add-volunteer-headshot-component branch March 29, 2017 21:08

Uh oh!

Conversation

WenInCode commented Nov 27, 2016

What's in this PR?

Changes

Todo

References

Uh oh!

joshsmith commented Nov 27, 2016

Uh oh!

WenInCode commented Nov 27, 2016

Uh oh!

joshsmith commented Nov 27, 2016

Uh oh!

joshsmith commented Nov 27, 2016

Uh oh!

joshsmith commented Nov 30, 2016

Uh oh!

WenInCode commented Nov 30, 2016

Uh oh!

WenInCode commented Dec 1, 2016

Uh oh!

joshsmith commented Dec 2, 2016

Uh oh!

WenInCode commented Dec 4, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

marineb commented Dec 4, 2016

Uh oh!

sbatson5 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

WenInCode commented Dec 5, 2016

add the remote

fetch the changes

checkout branches

push changes

Uh oh!

WenInCode commented Dec 5, 2016

Uh oh!

marineb commented Dec 5, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

WenInCode commented Dec 4, 2016 •

edited

Loading