Show committer avatar as well as author in the PR commit view

[z0al]

Fixes #774

Todo list

• Retrieve committer info
• Show committer's avatar in the commit view
• Fix design (e.g. pull avatars to the right as GitHub does)
• Add tests

[z0al]

I can't see why Travis is failing here. However, everything is OK in my fork Travis test result

[simurai]

Fix design (e.g. pull avatars to the right as GitHub does)

Switched to a table layout so that the avatars line up. Example:

Here another PR to test: atom/atom#13453

I can't see why Travis is failing here.

My commit seemed to fix it, but the failing was probably unrelated.

[z0al]

Switched to a table layout so that the avatars line up

@simurai I was trying to do the same locally, but I wouldn't make it perfect like your work, good CSS lesson ;)

but the failing was probably unrelated.

Good to know, thanks

[z0al]

@simurai Oops, looks like we broke something, just noticed it when testing against atom/atom#13453

I will fix it this time 😄

[kuychaco]

Yeah, apologies about the Travis failure. We have some flakey tests. We can restart the failing job for you if needed

Thanks for taking this on :)

[z0al]

I would like to add some tests for Commit container, but I can't figure out how!

I tried this code (after I dug into commits-container.test.js) to check how things work:

describe('CommitContainer', function() {
  it('renders a header with one user name', function() {
    const item = {
      author: {name: 'FirstName', avatarUrl: '', user: {login: 'FirstLogin'}},
      committer: {name: 'GitHub', avatarUrl: '', user: null},
      authoredByCommitter: true,
      oid: 'xxx',
      messageHeadline: ':smile: it\'s fun',
      messageHeadlineHTML: 'fake html',
    };
    const app = <Commit item={item} />;
    const instance = shallow(app);
    assert.match(instance.text(), /FirstLogin/);
  });
});

But the assertion fails with:

AssertionError: assert.match(instance.text(), /FirstLogin/): expected '<Octicon />xxx' to match /FirstLogin/

Any hint ? It's my first time to play with Relay and Enzyme

[z0al]

Ping @kuychaco :)

[BinaryMuse]

Sorry I haven't had a chance to dig into this — first day back after a holiday in the US.

I would like to add some tests for Commit container

I don't know if it's relevant to your failure or not, but the containers are responsible for reaching into the Relay store and such to fetch data from GraphQL. It's likely we'll want to separately export the non-containerized React component and test that directly instead.

[z0al]

Oh, my bad I meant Commit component.

It's likely we'll want to separately export the non-containerized React component and test that directly instead.

Isn't it already done in Commits component @BinaryMuse? Actually, I was trying to do the same with the Commit component.

I think I will give it another shot and see if I can manage it to work

Thanks

[BinaryMuse]

@ahmed-taj Ah I see, I'm sorry for the misinformation.

In the example you posted above, FirstLogin will never actually appear in the component's text because it's not rendered as text — it's rendered as an attribute of one of the images. For this test, you'll likely want to use one of Enzyme's other methods; here's a list of the ones provided by shallow instances: https://github.com/airbnb/enzyme/blob/master/docs/api/shallow.md

For example, to ensure that there's an image with the title attribute 'FirstLogin', you could use:

assert.isTrue(instance.containsMatchingElement(<img title="FirstLogin" />));

[z0al]

I think it's ready for review now!

[BinaryMuse]

Fantastic job on this! The tests look ✨. Thank you so much for your contribution!

[BinaryMuse]

Nice! I actually had no clue this was how it worked.

I dug into the GItHub.com source code a bit and found that the committer name will be "GitHub Enterprise" if we're running on GitHub enterprise. The noreply address is also based off the domain of the Enterprise install. I'm going to see if I can get a field added called authoredViaWeb or something that matches the internal check we do, but until then this works great. 👍

[z0al]

I'm going to see if I can get a field added called authoredViaWeb or something that matches the internal check we do

This would be perfect 👍