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 tabs for issueish panel items #1684

Merged
merged 72 commits into from Sep 25, 2018

Conversation

4 participants
@annthurium
Copy link
Contributor

annthurium commented Sep 7, 2018

Description of the Change

This change moves some of the information from the pull request view into tabs.

Screenies

screen shot 2018-09-20 at 1 53 39 pm

screen shot 2018-09-20 at 1 53 52 pm

docked:
screen shot 2018-09-20 at 3 06 26 pm

TODO

  • Put a border around the active/selected tab
  • get some input from team about tab content / ordering, make decision
  • Add octicons to tabs
  • Make the tabs render accordion style when the pane is docked.
  • Add unit tests
  • Investigate whether only the active tab should render first for performance reasons.
  • add gifs and screenshots to pr body
  • add content to Commits tab.
  • Clean up issue rendering
  • handle pagination for PRs with >100 commits

Alternate Designs

  • We had some back and forth about what information goes on each tab. @simurai's initial designs called for description, timeline, commits, and changes as the tab items. We had some back and forth about whether it might be better to use consistent language with what's used on dotcom. See the issue for details. Ultimately it would be best to validate our assumptions with uxr before we ship.

  • I could have rolled my own tabs instead of using a lib. Although it sounded like fun, I thought it might be a more efficient use of time to not reinvent wheels. This lib is small and well maintained and uses good a11y practices.

Benefits

Separating the information into tabs gives us room for pull request reviews in the editor, which is where all this is headed.

Possible Drawbacks

It's possible that users will find the new UI confusing since we're moving information around.

Applicable Issues

#1656

Test plan

  • manually test rendering an issue to verify that there are no visual regressions.
  • unit tests for new PrCommitView component to verify that:
    • show / hide more buttons show and hide commit message body
    • show / hide more buttons are only rendered if there is a commit message body
    • renders link to dotcom, author name and avatar, commit headline, humanized time since the commit happened
  • unit tests for IssueishDetailView to test that it renders 3 tabs with the right text and icons
  • unit tests for PrCommitsView to verify that:
    • n commits are rendered if n commits are passed in
    • load more button is only rendered if there's more commits to be loaded
    • clicking loadMore calls the function that loads more stuff

TODOs for future prs:

  • display co-author avatars
  • refactor header rendering in IssueishDetailView to improve readability
  • figure out why keyboard nav doesn't work and fix it

annthurium added some commits Sep 7, 2018

Move some content into tabs
You get a tab.  And you get a tab.  Everybody gets a tab!
🔥 bullets
I just wanted to use that commit message, ok?  Don't judge meeee.
move timeline to conversation tab
this is more consistent with how this content is displayed on github.com

annthurium added some commits Sep 8, 2018

@coveralls

This comment has been minimized.

Copy link

coveralls commented Sep 8, 2018

Coverage Status

Coverage increased (+0.07%) to 82.006% when pulling f88ab19 on tt-18-sept-tabs-tabs-tabs into 10d4dd2 on master.

@smashwilson smashwilson referenced this pull request Sep 11, 2018

Closed

Multi-File Diff #1689

annthurium and others added some commits Sep 11, 2018

fetch commit data with relay
Co-Authored-By: Katrina Uychaco <katrina@github.com>
rendering some commits, woo
Co-Authored-By: Katrina Uychaco <katrina@github.com>
make it slightly less ugly
and also add a really long commit body so I can test what that looks 
like 
asdkfjksajfklsdajfldsajfklsadjflskjfljdsklajfklsdajfkldsjfkldsajflkdasjfkldsjkldsjflksdajfdksklfdsaj

@annthurium annthurium changed the title [wip] Add tabs for issueish panel items Add tabs for issueish panel items Sep 20, 2018

@annthurium annthurium requested a review from kuychaco Sep 20, 2018

@annthurium

This comment has been minimized.

Copy link
Contributor Author

annthurium commented Sep 20, 2018

@kuychaco, as far as what kind of feedback I'm looking for in code review:

  • did I follow all the graphql conventions we've set in the package? Or are there things that could be improved?
  • Are there ways I could refactor to enhance readability?
@simurai

This comment has been minimized.

Copy link
Member

simurai commented Sep 21, 2018

Something that came to mind when looking at the many commits of #1512 in the issueish pane: Should the order be reversed? So that new commits appear at the top and you don't have to scroll down and potentially click on "load more" a few times, just to see if new commits are made. The timeline also has the same issue.

We don't have to change it now, just something to consider one day.

Style "Load more" button in the Overview
To match the commits.
<PrTimelineContainer
{...childProps}
switchToIssueish={this.props.switchToIssueish}
/> :

This comment has been minimized.

@simurai

simurai Sep 21, 2018

Member

There is a : at the bottom left in the Overview tab:

screen shot 2018-09-21 at 9 31 58 pm

Is it the ☝️ above : that could be removed?

This comment has been minimized.

@annthurium

annthurium Sep 21, 2018

Author Contributor

oh, that's a mistake! Thanks for pointing it out. Will remove.

Feature Sprint : 27 August - 14 September 2018 : v0.20.0 automation moved this from In Progress 🔧 to QA Review 🔬 Sep 21, 2018

@kuychaco
Copy link
Member

kuychaco left a comment

This all looks great! The code as well as the finished product. Well done!

Show resolved Hide resolved lib/views/issueish-detail-view.js
switchToIssueish={this.props.switchToIssueish}
/>
}
{isPr ? this.renderPullRequestBody(issueish, childProps) : this.renderIssueBody(issueish, childProps)}

This comment has been minimized.

@kuychaco

kuychaco Sep 21, 2018

Member

👍 nice nice

<div className="github-PrTimeline-load-more-link" onClick={this.loadMore}>
{this.props.relay.isLoading() ? <Octicon icon="ellipsis" /> : 'Load More'}
<div className="github-PrTimeline-loadMore">
<button className="github-PrTimeline-loadMoreButton btn" onClick={this.loadMore}>

This comment has been minimized.

@kuychaco

kuychaco Sep 21, 2018

Member

👍 on this being a button

edges {
cursor
node {
commit {

This comment has been minimized.

@kuychaco

kuychaco Sep 21, 2018

Member

Let's extract this as a fragment on PrCommitView. See timeline/commits-view.js and timeline/commit-view.js as an example.

This keeps our query fragments co-located with the views that use the resulting data, which is inline with Relay's philosophy (reducing unnecessary coupling, reasoning about discrete units of an application in isolation, and breaking complex interfaces into reusable components). I think this may also allow relay to do some extra checking for us to make sure that data from our queries match with what is actually being used.

@@ -161,6 +163,7 @@ describe('check out a pull request', function() {
});
}

// achtung! this test is flaky

This comment has been minimized.

@kuychaco

kuychaco Sep 21, 2018

Member

hmm weird. Any idea why? 🤔

@@ -109,7 +109,7 @@ describe('IssueishTimelineView', function() {
const wrapper = shallow(buildApp({
relayHasMore: () => false,
}));
assert.isFalse(wrapper.find('.github-PrTimeline-load-more-link').exists());
assert.isFalse(wrapper.find('.github-PrTimeline-loadMoreButton').exists());

This comment has been minimized.

@kuychaco

kuychaco Sep 21, 2018

Member

Not sure about the appropriate convention here, just noticing that we've switched from hyphen-separated to camelCase... same below.

This comment has been minimized.

@annthurium

annthurium Sep 21, 2018

Author Contributor

I think @simurai made that change - I'm happy to switch to kabob-case for the sake of consistency.

This comment has been minimized.

@simurai

simurai Sep 21, 2018

Member

The naming convention would be camelCase for child elements of a component. So it can easily be read in groups: github-PrTimeline-loadMoreButton -> It's the loadMoreButton inside the PrTimeline component inside the github package. We don't follow that convention everywhere, which isn't a big deal.

committerName: 'Margaret Hamilton',
date: '2018-05-16T21:54:24.500Z',
messageHeadline: 'This one weird trick for getting to the moon will blow your mind 🚀',
abbreviatedOid: 'bad1dea',

This comment has been minimized.

@kuychaco
@kuychaco

This comment has been minimized.

Copy link
Member

kuychaco commented Sep 21, 2018

Should the order be reversed?

Oh oops. Yeah the order should totally be recent items first

annthurium and others added some commits Sep 21, 2018

change "link" to "button" in IssueishTimelineView tests
Co-Authored-By: Katrina Uychaco <katrina@github.com>
add tests for PrCommitView
Co-Authored-By: Katrina Uychaco <katrina@github.com>
add tests for loadMoreButton
Co-Authored-By: Katrina Uychaco <katrina@github.com>
@annthurium

This comment has been minimized.

Copy link
Contributor Author

annthurium commented Sep 21, 2018

@kuychaco @simurai dotcom renders the newest commits at the bottom of the commits tab. While we don't have to stay consistent with dotcom in every way, I think that reversing the order here might be confusing to users. I'm willing to test this with uxr and revisit it, though!

kuychaco and others added some commits Sep 21, 2018

🔥 .only
I knew I was going to do that.

Co-Authored-By: Katrina Uychaco <katrina@github.com>

@kuychaco kuychaco force-pushed the tt-18-sept-tabs-tabs-tabs branch from f07bb6a to 4d2e7f1 Sep 24, 2018

@annthurium annthurium merged commit 1b4c899 into master Sep 25, 2018

6 checks passed

ci/circleci: beta Your tests passed on CircleCI!
Details
ci/circleci: dev Your tests passed on CircleCI!
Details
ci/circleci: stable Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.07%) to 82.006%
Details

Feature Sprint : 27 August - 14 September 2018 : v0.20.0 automation moved this from QA Review 🔬 to Merged ☑️ Sep 25, 2018

@annthurium annthurium referenced this pull request Oct 20, 2018

Closed

v0.21.0-0 QA Review #1752

7 of 7 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.