-
Notifications
You must be signed in to change notification settings - Fork 54
Ui modernization #772
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
base: 2027
Are you sure you want to change the base?
Ui modernization #772
Conversation
2e89356
to
d6dd638
Compare
d6dd638
to
30d1cca
Compare
* Remove simple pathUtils functions * Remove unused deps * Fix extension build * Delete unused stuff, do formatting touch ups * Fold wpilibapi into extension and remove shims * Move i18n to l10n and fix API imports * Remove unneeded project generator code * Fix 2025 stuff * Fix CSS
* Move catch block * Remove Command Palette vendordep manager * Make webpack configs work properly * Fix up help page layout * Fix icon * Remove unnecessary .then() * Format
…ation link on help page to 2027
* Clean up utilities * Inline file processor * Format
This looks to be doing multiple things, which makes it really hard to review. It looks like there's refactoring that's enabled by #767, which should probably be a separate PR, so that the actual UI changes are more easily reviewable. Also, it would really help for the PR to describe how the UI is improved. A picture is worth a thousand words. |
* Move dependencyView back to its original location * Move files back * Move locale file back
We moved several files back to their original locations to help minimize the diff. The API package folding and new utility files were not undone, solely because this originated as an deep refactoring of many dark corners of the codebase, and the changes are a little too intertwined to safely undo. This branch has already been shredded apart and reconstituted multiple times to get it in a state even remotely close to review. The codebase is fundmentally under too much tech debt to be able to do smaller incremental refactors, and we have several deeper refactors queued up that are blocked on the changes in this PR. The diff on just the TypeScript files is now +2975 lines, -1769 lines, split between modernizing the UI (the majority) and refactoring file handling for project generation and file templates. We apologize for the large diff, but hope that the PR is still reviewable enough that reviewed and merged. If it helps with reviewing, everything in shared and the commands.ts files for both C++ and Java, except for the vendor files in shared are the files that were changed because of the file handling refactor. Everything else is either minor changes like imports or typo fixes, or it's UI modernization. |
Absolutely! Starting small, this PR adds an "install from url" section to the vendor dependencies UI that was added in 2025. In adding this, the command palette vendordep manager is officially removed as it is no longer needed for anything. Also, it revamps the RioLog UI to be cleaner, use VSCode UI elements and styling (including the user's VSCode theme) and adding ANSI escape sequence support so that the color of log entries can be set as they would in a normal terminal. It also adds a default "welcome" message when initialized. The WPILib Help page has been updated and now includes some basic getting started tasks and a button that opens the command palette. The project creation page has been updated to be paginated, make use of the user's VSCode theme and also make use of VSCode's inbuilt UI elements. And lastly the project importer has also been updated to follow a similar design language If there's anything else you'd like to know, I (or @Gold856) would be happy to answer! |
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'll try to find some more time this evening, but here's a few things I noticed at a quick glance.
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 think I've managed to look over all this now. Got a few more comments.
* Undo changes to VendorLibraries to fix vendordep manager not updating * Format
<div class="wizard-step" id="step-3"> | ||
<div class="step-header"> | ||
<h2>Step 3: Review & Import</h2> | ||
<p>Review your settings and select additional options before importing.</p> |
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.
What other options can be selected? I don't see any
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.
That refers to the source, destination, and team number. I'm open to alternate wording
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.
Shouldn't it just be review your settings before importing?
- Bump VSCode engine requirement to ^1.90.0 - Update various package dependencies including: - @types/node from ^20.19.0 to ^20.19.11 - @types/node-fetch from ^2.6.12 to ^2.6.13 - @typescript-eslint/eslint-plugin and parser from ^8.34.0 to ^8.42.0 - eslint from ^9.28.0 to ^9.34.0 - ts-loader from ^9.5.2 to ^9.5.4 - typescript from ^5.8.3 to ^5.9.2 - webpack from ^5.99.9 to ^5.101.3 - Suggested changes from @sciencewhiz's review
<div class="project-field-container"> | ||
<input id="teamNumber" type="text" inputmode="numeric" class="vscode-textfield" required /> | ||
<label for="teamNumber">Team Number</label> | ||
<input id="teamNumber" class="vscode-textfield" type="number" inputmode="numeric" required /> |
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'm confused why we're changing this back to <input type="number">
. Per #772 (comment) it seemed like we didn't want the spinners in the first place, but this commit now adds the spinners?
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.
Your suggested change from your last review looked to me like a re-adding of spinners. Originally the idea was to have input type number so we could validate that it was indeed a valid team number, but hide the spinners with css
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 figured after I left #772 (comment) and still saw it there after I took another look, you just hadn't bothered to remove the unused CSS without a button to press to delete it
I tried to add a vendordep from URL. I used this URL: |
I've just checked and it works on https://github.com/nobody5050/vscode-wpilib/tree/nobody5050/improvements but does not seem to work on this branch. Looks like somehow while trimming down the changes to make the pr easier to review we broke it. I'll take a look sometime tonight and push up a fix for that. |
closes #23