Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upRefactor dialog handling #2209
Refactor dialog handling #2209
Conversation
codecov
bot
commented
Jul 17, 2019
•
Codecov Report
@@ Coverage Diff @@
## master #2209 +/- ##
==========================================
+ Coverage 92.71% 92.75% +0.03%
==========================================
Files 216 219 +3
Lines 12347 12354 +7
Branches 1815 1798 -17
==========================================
+ Hits 11448 11459 +11
+ Misses 899 895 -4
Continue to review full report at Codecov.
|
|
Self-review for context. |
| @@ -29,7 +29,7 @@ We move focus around by registering Atom commands. | |||
| For example, in `GitTabView`: | |||
|
|
|||
| ``` | |||
| this.props.commandRegistry.add(this.refRoot, { | |||
| this.props.commands.add(this.refRoot, { | |||
This comment has been minimized.
This comment has been minimized.
smashwilson
Jul 19, 2019
Author
Member
This is a renaming I slipped in just because it was annoying me
| editor.getElement().classList.add(this.props.className); | ||
| } | ||
| if (this.props.preselect) { | ||
| editor.selectAll(); |
This comment has been minimized.
This comment has been minimized.
smashwilson
Jul 19, 2019
Author
Member
I went back and forth between supporting "all text preselected" and "cursor positioned at the end of the provided buffer." I think I landed on "all text preselected" because it's slightly more flexible (one keypress to decline a suggested starting value!) and because it's slightly easier to code (I don't have to pull anything from the buffer here.)
| @@ -22,14 +22,12 @@ export default class Panel extends React.Component { | |||
| children: PropTypes.element.isRequired, | |||
| options: PropTypes.object, | |||
| onDidClosePanel: PropTypes.func, | |||
| visible: PropTypes.bool, | |||
This comment has been minimized.
This comment has been minimized.
| * ``` | ||
| * | ||
| */ | ||
| export default class AutoFocus { |
This comment has been minimized.
This comment has been minimized.
smashwilson
Jul 19, 2019
Author
Member
I'm wondering if I could generalize this somehow to deal with #1403. It'd likely need to wait on a hooks refactor, though, and be passed as a context...
| return null; | ||
| } | ||
|
|
||
| class DialogRequest { |
This comment has been minimized.
This comment has been minimized.
smashwilson
Jul 19, 2019
Author
Member
This class is in here so that I can restrict the scope of "things that need to know the List Of All Possible Dialogs" to only this file. Hopefully that'll make it less error-prone to add new ones in the future.
| initDialogPath: null, | ||
| initDialogResolve: null, | ||
| credentialDialogQuery: null, | ||
| dialogRequest: dialogRequests.null, |
This comment has been minimized.
This comment has been minimized.
smashwilson
Jul 19, 2019
Author
Member
One of the big reasons I wanted to do this was to make this state less of a "blob of everything."
| <Command command="github:toggle-git-tab" callback={this.gitTabTracker.toggle} /> | ||
| <Command command="github:toggle-git-tab-focus" callback={this.gitTabTracker.toggleFocus} /> | ||
| <Command command="github:toggle-github-tab" callback={this.githubTabTracker.toggle} /> | ||
| <Command command="github:toggle-github-tab-focus" callback={this.githubTabTracker.toggleFocus} /> | ||
| <Command command="github:clone" callback={this.openCloneDialog} /> | ||
| <Command command="github:open-commit" callback={this.showOpenCommitDialog} /> | ||
| <Command command="github:initialize" callback={() => this.openInitializeDialog()} /> |
This comment has been minimized.
This comment has been minimized.
smashwilson
Jul 19, 2019
Author
Member
Oh hey github:initialize is new. I am the worst at "refactoring"
|
|
||
| const atomGitRepo = await this.project.repositoryForDirectory(directory); | ||
| if (atomGitRepo) { | ||
| await atomGitRepo.refreshStatus(); |
This comment has been minimized.
This comment has been minimized.
smashwilson
Jul 19, 2019
Author
Member
So this doesn't actually prompt Atom's tree-view to recompute workdir's directory icon to reflect that it's now a git repository. I did a little poking in tree-view source to see if that was possible and wasn't able to come up with anything.
…actor-dialogs
smashwilson commentedJul 17, 2019
•
edited
Please be sure to read the contributor's guide to the GitHub package before submitting any pull requests.
Requirements
Description of the Change
To prepare for the implementation of #2111, I'm refactoring and tidying up some of the code related to dialog handling.
Our dialogs are currently managed by
RootController. All of their state is captured in common-prefix RootController state, including the "is this dialog visible right now or not" logic. RootController is already a bit overwhelming, so before I add more dialogs, I'm extracting the dialog-handling code to its ownDialogsController.Screenshot/Gif
N/A
Alternate Designs
N/A
Benefits
Hopefully
RootControllerwill be slightly more readable.Possible Drawbacks
N/A
Applicable Issues
N/A
Metrics
N/A
Tests
Visual inspection of:
Documentation
N/A
Release Notes
N/A
User Experience Research (Optional)
N/A
View rendered docs/focus-management.md