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

web reporter refactor and issueFormService #212951

Merged
merged 29 commits into from
May 24, 2024

Conversation

justschen
Copy link
Contributor

@justschen justschen commented May 17, 2024

more refactoring and adding in web functionality.

@bpasero bpasero self-requested a review May 22, 2024 16:06
@bpasero
Copy link
Member

bpasero commented May 22, 2024

Adding myself for reviewing overall structure and layering of the code, not so much deep detail of it.

Copy link
Contributor

@andreamah andreamah left a 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!

@justschen justschen marked this pull request as ready for review May 22, 2024 17:09
@VSCodeTriageBot VSCodeTriageBot added this to the May 2024 milestone May 22, 2024
Copy link
Member

@bpasero bpasero left a 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.

src/vs/workbench/workbench.web.main.ts Outdated Show resolved Hide resolved
Copy link
Member

@bpasero bpasero left a 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/workbench.web.main.ts Outdated Show resolved Hide resolved
src/vs/workbench/services/issue/browser/issueService.ts Outdated Show resolved Hide resolved
build/gulpfile.vscode.js Outdated Show resolved Hide resolved
src/vs/workbench/workbench.common.main.ts Outdated Show resolved Hide resolved
src/vs/workbench/workbench.desktop.main.ts Outdated Show resolved Hide resolved
Copy link
Member

@bpasero bpasero left a 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/workbench.common.main.ts Outdated Show resolved Hide resolved
src/buildfile.js Show resolved Hide resolved
bpasero
bpasero previously approved these changes May 24, 2024
Copy link
Member

@bpasero bpasero left a 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.

@justschen justschen merged commit d05d280 into microsoft:main May 24, 2024
6 checks passed
andremmsilva pushed a commit to PIC1G55/vscodeG55 that referenced this pull request May 26, 2024
* 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:
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants