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.

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 avatar and split up TabHeaderView #2325

Merged
merged 46 commits into from Nov 18, 2019
Merged

Conversation

@wadethestealth
Copy link
Contributor

wadethestealth commented Oct 29, 2019

Description of the Change

Canonical Pr: #2308

This is an MVP of the avatar, and in a following Pr I will add the onClick actions, however show name hovering is supported via the title tag on the avatar img tag.

  • Updates the Author model to contain an avatarUrl
  • Splits Tab Header into a git and github tab header (with container for github and controller for both)
    • Adds avatar to git tab
    • Adds avatar to github tab

Note: currently on the github tab header if the query fails it uses a null user until the component is rerendered, and it does not provide a method for retrying after the original query fails. Same for invalid tokens too.

Note: this currently implements a separate query for the GitHub avatar. A sibling component (Remote container) also requests a query and I am wondering about if we and how in terms of component structure and naming one should combine those queries.

See Tests for testing changes.

Screenshot/Gif

Untitled-1

Benefits

This gives a visual identity for users to understand what git email they are using for committing.

Possible Drawbacks

n/a

Applicable Issues

Continuation of #2308 with respect to #1934

Tests

  • update previous Author tests
  • add new tests for git and github tab header views
  • add new tests for git and github tab header controllers
  • add new tests for github tab header container
@wadethestealth wadethestealth mentioned this pull request Oct 29, 2019
6 of 6 tasks complete
@wadethestealth wadethestealth force-pushed the wadethestealth:avatar branch from bba46c7 to eb0a1a4 Nov 2, 2019
@wadethestealth

This comment has been minimized.

Copy link
Contributor Author

wadethestealth commented Nov 2, 2019

@smashwilson I added in an avatar that only comes from the git config, but I think I have added to much state to the component compared to other components in this code base. I believe I now need to separate TabHeaderView into 5 parts.

GitTabHeaderController, which will handle the committer and workdir state changing (this is essentially the current TabHeaderView, but then roughly I take the render method and put it into the new GitTabHeaderView.

The other parts begin with GithubTabContainer, which queries for viewer: {name, email, avatarUrl, login}. Then, GithubTabController which is similar to the current TabHeaderView, but without worrying about the committer things since that will be handled by the new GithubTabContainer. Lastly, GithubTabHeaderView, which will be similar to the GitTabHeaderView, but it will also include an eventual refresh button.

As it stands, this PR looks nice, is ready, is passing (minus that flake), and accomplishes the task (except for using the github user, this only uses the git config user), but I feel like it doesn't fit into the code base, which I haven't used Ruby, but it seems like its a similar structure here.

Please confirm if you agree though. (Went ahead and began the change, but if necessary I can revert)

Also unrelated note, should I still only be mentioning you, as I have seen 2 other atom developers merge new changes to the master branch.

@wadethestealth wadethestealth mentioned this pull request Nov 3, 2019
0 of 3 tasks complete
@wadethestealth wadethestealth force-pushed the wadethestealth:avatar branch from f934a46 to 4a48429 Nov 6, 2019
@smashwilson

This comment has been minimized.

Copy link
Member

smashwilson commented Nov 7, 2019

[..] I believe I now need to separate TabHeaderView into 5 parts [..] Please confirm if you agree though.

💯 Yes, thank you!

The pieces you've described match exactly how we try to split up functionality among different kinds of components:

https://github.com/atom/github/blob/master/docs/react-component-classification.md#react-component-classification

Just out of curiosity, had you read that already, or did you pick this up from the existing codebase? 😇

Also unrelated note, should I still only be mentioning you, as I have seen 2 other atom developers merge new changes to the master branch.

I believe I still have the most history and context and Opinions ™️ about this specific package, but I tinker with this (and respond to comments) in my spare time. @lkashef and @darangi are more reliably engaged across the Atom org, so they'll likely be faster to respond to direct pings, but are working across a huge number of repos and won't have as deep knowledge about this specific package. All three of us have merge and release rights though.

Really you shouldn't have to ping anyone specifically though... I'm watching the repo, and I tend to shelve "things to review / respond to" until I get the chance to go through and respond to a bunch at once.

@wadethestealth

This comment has been minimized.

Copy link
Contributor Author

wadethestealth commented Nov 7, 2019

Just out of curiosity, had you read that already, or did you pick this up from the existing codebase?

I read it during my first pull request but shelved it because it said views could have some state and I didn't understand why you had it separated. Ironically I was going through ruby's official tutorial/interactive playground, and it mentioned this architecture, and instantly i was like "Github uses ruby", "atom github is made by github, most of them probably know ruby", "ooooooh they are using this idea but just in js". But also in this PR the file grew clearly way to large, and it looked nothing like the code base when I was referencing how things are already done.

@wadethestealth wadethestealth changed the title Add avatar to TabHeaderView Add avatar and split up TabHeaderView Nov 14, 2019
Copy link
Member

smashwilson left a comment

Nice. The only merge-blocker I see is those .skip( clauses in some of the tests.

lib/controllers/git-tab-header-controller.js Outdated Show resolved Hide resolved
lib/controllers/github-tab-header-controller.js Outdated Show resolved Hide resolved
lib/git-shell-out-strategy.js Show resolved Hide resolved
lib/views/git-tab-header-view.js Outdated Show resolved Hide resolved
lib/views/recent-commits-view.js Show resolved Hide resolved
test/views/github-tab-header-view.test.js Outdated Show resolved Hide resolved
test/views/github-tab-header-view.test.js Outdated Show resolved Hide resolved
test/views/github-tab-header-view.test.js Outdated Show resolved Hide resolved
@wadethestealth wadethestealth requested a review from smashwilson Nov 18, 2019
@smashwilson smashwilson merged commit f1d164a into atom:master Nov 18, 2019
6 checks passed
6 checks passed
atom.github Build #20191118.1 succeeded
Details
atom.github (Lint) Lint succeeded
Details
atom.github (Linux) Linux succeeded
Details
atom.github (MacOS) MacOS succeeded
Details
atom.github (Snapshot) Snapshot succeeded
Details
atom.github (Windows) Windows succeeded
Details
@wadethestealth wadethestealth deleted the wadethestealth:avatar branch Nov 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.