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

open Git pane when merge conflicts are present #1709

Merged
merged 2 commits into from Sep 28, 2018

Conversation

Projects
None yet
2 participants
@annthurium
Contributor

annthurium commented Sep 27, 2018

Description of the Change

When I was working on #1704, I noticed ensureGitTabVisible is never called.

Doing a bit of code archaeology, it looks like we used to pop open the git tab automatically when merge conflicts were present. I don't think we intentionally removed this functionality, so let's bring it back.

My reasons for wanting to revive this functionality are anecdotal: an old coworker of mine discovered the git tab because it popped open when there were merge conflicts.

Alternate Designs

none considered.

Benefits

Users can resolve merge conflicts more easily within Atom since the git tab is already open. It might improve discoverability of the git tab by showing it to the user at an opportune moment.

Possible Drawbacks

Users who don't want to resolve their merge conflicts in Atom might be annoyed about the git pane opening.

Applicable Issues

#1705

Metrics

Instrumenting when the git tab opens doesn't tell us much, since we're doing that automatically. I'm curious if users are using the git tab after it's open, but we don't have "funnel" instrumentation that easily allows us to track which actions were taken immediately before. Thus, I'm not adding any metrics instrumentation to this pull request.

Tests

  • unit tests to verify that ensureGitTabPresent is called when merge conflicts are present.
  • manual testing to verify that the git tab pops open when merge conflicts are present.
@@ -40,6 +40,8 @@ describe('StatusBarTileController', function() {
notificationManager={notificationManager}
tooltips={tooltips}
confirm={confirm}
toggleGitTab={() => {}}

This comment has been minimized.

@annthurium

annthurium Sep 27, 2018

Contributor

I noticed that there were some console warnings about these missing props, when I ran the unit tests.

It's so easy to miss these kinds of warnings, because running the full suite takes so long that I don't usually do it. :-( I don't know what to do about that though.

@coveralls

This comment has been minimized.

coveralls commented Sep 27, 2018

Coverage Status

Coverage increased (+0.02%) to 82.024% when pulling 1a86933 on tt-18-sept-unused-prop into 157c5a8 on master.

@annthurium annthurium merged commit 7950c43 into master Sep 28, 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.02%) to 82.024%
Details

@annthurium annthurium deleted the tt-18-sept-unused-prop branch Sep 28, 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