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

Make CommitView use the Command component #1539

Merged
merged 1 commit into from Jun 20, 2018

Conversation

Projects
2 participants
@annthurium
Copy link
Contributor

annthurium commented Jun 20, 2018

As per #1404 we should be using the Command component instead of registering commands directly. I believe this is the last place where we're not doing so.

manual testing performed:

  • making a new commit
  • amending the most recent commit commit
  • toggling the expanded commit editor
  • adding a new co author (including tabbing between fields, and using escape to close the dialog)

Also, ran all the unit tests to ensure they passed.

I found some bugs with the co author flow while doing this manual testing, but these bugs were reproducible on a clean build synced to master and thus are unrelated to these changes. I filed them as separate issues.

@annthurium annthurium requested a review from smashwilson Jun 20, 2018

@smashwilson
Copy link
Member

smashwilson left a comment

👍 One more little inconsistency out of the way. Thanks 🙇

/>
</Commands>
<Commands registry={this.props.commandRegistry} target="github-CommitView-coAuthorEditor">
<Command command="github:co-author:down" callback={this.proxyKeyCode(40)} />

This comment has been minimized.

@smashwilson

smashwilson Jun 20, 2018

Member

One thing I've noticed is that the command palette doesn't handle the double-colon command form very well: these end up displaying as undifferentiated github: co-author entries. (See #1407 for the original bug report.) I "fixed" it before by targetting these commands to the co-author editor so they won't appear elsewhere, but maybe we should still change them to be single-colon instead (github:co-author-down) for consistency anyway?

Not as part of this PR though 😄

@annthurium annthurium merged commit 5a6813d into master Jun 20, 2018

3 checks passed

ci/circleci 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

@annthurium annthurium deleted the tt-18-jun-command branch Jun 20, 2018

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.