-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
PR on Start Work operation in Home view (#3621) #3698
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly nitpicks - the only major suggestion would be to set up a provider that manages the issue querying and includes caching and error handling as we have with Launchpad. Most of this would be for future-proofing purposes - we don't automatically query for issues with this feature like we do for Launchpad - but the need for that may arise sooner than we think (for example - some UX on the Home view showing a count of how many issues we have, etc.)
src/commands/quickCommand.steps.ts
Outdated
0, | ||
0, | ||
createDirectiveQuickPickItem(Directive.Noop, undefined, { | ||
label: 'Start work lets you quickly create a branch and initial code for a selected issue', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick, but we might want to mention worktrees in here somehow since this flow will, most likely, at least heavily encourage creating one.
src/git/gitProviderService.ts
Outdated
@@ -776,7 +776,7 @@ export class GitProviderService implements Disposable { | |||
return { allowed: subscription.account?.verified !== false, subscription: { current: subscription } }; | |||
} | |||
|
|||
if (feature === 'launchpad') { | |||
if (feature === 'launchpad' || feature === 'startWork') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might want to follow up with @justinrobots if you haven't already since I think there was some confusion on whether this gating applies to "Start Work" the same way it does for Launchpad.
src/plus/startWork/startWork.ts
Outdated
}; | ||
|
||
export const StartWorkQuickInputButton: QuickInputButton = { | ||
iconPath: new ThemeIcon('beaker'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I put the 'beaker' icon as a placeholder in my WIP, but we should probably check in with the team on what would make a good representative icon for this feature in general, as well as for actions like this.
src/plus/startWork/startWork.ts
Outdated
async function updateContextItems(container: Container, context: Context) { | ||
context.result = { | ||
items: | ||
(await container.integrations.getMyIssues([HostingIntegrationId.GitHub]))?.map(i => ({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps we can do one of the following:
- Create a
startWorkProvider
- Rename the
launchpadProvider
to be a bit more flexible outside the domain of Launchpad itself and extend it
And then we can add functions to the provider to:
- Get searched issues, with caching and error handling like we do for Launchpad items, and the ability to filter out stale ones and other post-processing like we do with PRs for Launchpad (not that we have to implement any post-processing now, just the caching and error handling parts)
- Complete actions on "start work" items, like creating a branch, etc.
- Set up other utilities that we want in the future for Start Work so that the command file doesn't get out of hand.
- Use those functions here as needed, like
launchpad
does withlaunchpadProvider
.
a805e00
to
2700c4a
Compare
51622b8
to
937b1cc
Compare
Please review it one more time. All notes related to implementing a provider haven't been fixed yet, because I've been focusing on the functionality and Eric has suggested to implement optimizations as a follow-up. So, now you're welcome to suggest any urgent fixes that I can make on Monday. |
@sergeibbb I assume you mean @eamodio instead of me, right? |
True. Sorry for bothering |
21890a0
to
8f37ef6
Compare
ce443ae
to
420fde8
Compare
disappears for me in pre-releas. It disappears every time when the window looses the focus, but it stays if (and after) the quickpick has a text in the search field. quickpick-disappears.mp4But I agree that Start Work is different from Launchpad. After typing something in the search field and clicking the globe: Launchpad stays, but Start Work disappears. I'm going to fix it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some of the issues from previous review may be due to debugging or pre-existing regressions. We will address those and any other unaddressed issues above post-merge.
Description
This PR introduces initial Start Work flow that:
Checklist
Fixes $XXX -
orCloses #XXX -
prefix to auto-close the issue that your PR addresses