Refactor dialog handling #2209
Refactor dialog handling #2209
Conversation
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.
|
smashwilson
left a comment
smashwilson
left a comment
There was a problem hiding this comment.
Self-review for context.
|
|
||
| ``` | ||
| this.props.commandRegistry.add(this.refRoot, { | ||
| this.props.commands.add(this.refRoot, { |
There was a problem hiding this comment.
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(); |
There was a problem hiding this comment.
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.)
| children: PropTypes.element.isRequired, | ||
| options: PropTypes.object, | ||
| onDidClosePanel: PropTypes.func, | ||
| visible: PropTypes.bool, |
There was a problem hiding this comment.
🔥 Unused props from waaaay back in the day.
| * ``` | ||
| * | ||
| */ | ||
| export default class AutoFocus { |
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
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, |
There was a problem hiding this comment.
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-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()} /> |
There was a problem hiding this comment.
Oh hey github:initialize is new. I am the worst at "refactoring"
|
|
||
| const atomGitRepo = await this.project.repositoryForDirectory(directory); | ||
| if (atomGitRepo) { | ||
| await atomGitRepo.refreshStatus(); |
There was a problem hiding this comment.
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.
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