-
Notifications
You must be signed in to change notification settings - Fork 30.7k
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
web reporter refactor and issueFormService #212951
Conversation
Adding myself for reviewing overall structure and layering of the code, not so much deep detail of 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.
Did a quick review- I haven't done much CSS in this repo, so this is just a cursory look. Nice work!
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 only glanced at the layer structure and I like the direction this is going but I see a future in where the issue reporter - even on desktop - is fully operated and opened from the workbench (via window.open
). This would help ensuring we are using the same solution on all platforms and probably allows for even more code reuse. I suggest to do that move as a follow up next month 👍
As for where all the issue related files go that are currently in vs/workbench/issue
, I would suggest to move them into the existing vs/workbench/contrib/issue
folder (into the respective browser
and electron-sandbox
folders there). The issue reporter is imho a contribution to the workbench and not otherwise used by core services, so I think it qualifies for the contrib
layer. Even though its weird to have a contrib
being bundled in our build files, this would go away once we remove all electron-main
pieces.
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.
Why do we need to keep some of the files in workbench/electron-sandbox/issues
? They belong into the contrib/issue
folder as well, no?
src/vs/workbench/services/extensionManagement/browser/extensionBisect.ts
Outdated
Show resolved
Hide resolved
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.
Pretty close now for me at least.
src/vs/workbench/services/extensionManagement/browser/extensionBisect.ts
Outdated
Show resolved
Hide resolved
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.
Good from my end, pending the verification in a real build.
* web version working * change to mainWindow * PROPER MOVEMENT * working for web as well * move issueFormService to workbench/contrib/issue * cleaning up{ * more cleanup, added setting * styling * use mainwindow to open and closee * css fixes * fix css again * fix CSS and wonky applyCSS rules * change gulpfile * add and update system info * address some of the comments * move files! small changes * move JS and non window specific back to electron sandbox * fix on issueReporter.js * fix build file * fix gulp file too.... * move everything into contrib * fix workbench import * move everything else into contrib, fix import * change name to web * applying more feedback fixes :D * fix command and remove unused import: * add back issueTroubleshoot * fix gulpile outputs * fix out exclusion:
more refactoring and adding in web functionality.