-
Notifications
You must be signed in to change notification settings - Fork 79
[feature] new project explorer #7592
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: main
Are you sure you want to change the base?
Conversation
… logic was not taking parent into account
9c7506c
to
ccc7ec9
Compare
@@ -87,38 +87,3 @@ jobs: | |||
git commit -am "Look at this (photo)Graph *in the voice of Nickelback*" || true | |||
git push | |||
git push origin ${{ github.head_ref }} | |||
|
|||
npm-test-unit-components: |
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.
This is deleted because the billing.test.tsx
is covered in vitest which is the npm run test:unit
workflow. Its all one runner for multiple file types.
@@ -345,12 +337,12 @@ test.describe('when using the file tree to', () => { | |||
await test.step('swap between small and large files', async () => { | |||
await openDebugPanel() | |||
// Previously created a file so we need to start back at main.kcl | |||
await mainFile.click() | |||
await toolbar.openFile('main.kcl') |
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.
toolbar.openFile
does the click
for me. I updated these lines to use the helper function.
const renamedFile = page.getByRole('listitem').filter({ | ||
has: page.getByRole('button', { name: newFileName + FILE_EXT }), | ||
}) | ||
const fileToRename = u.locatorFile('fileToRename.kcl') |
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.
In many locations there were specific page.getByRole
calls, I've moved this to helper functions that still provide the Locator
but now there are two functions to maintain for folder and file. This is a lot better.
await page.keyboard.press('Enter') | ||
}) | ||
}, | ||
|
||
cloneFile: async (name: string) => { | ||
return test?.step(`Cloning file '${name}'`, async () => { | ||
await page | ||
.locator('[data-testid="file-pane-scroll-container"] button') | ||
.locator('[data-testid="file-pane-scroll-container"] [role=treeitem]') |
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.
The DOM element is not a button anymore, I can use role=treeitem
instead. This is a reasonable change across the helper functions.
@@ -131,8 +131,7 @@ | |||
"test": "vitest --mode development", | |||
"test:rust": "(cd rust && just test && just lint)", | |||
"test:snapshots": "cross-env TARGET=web NODE_ENV=development playwright test --config=playwright.config.ts --grep=@snapshot --trace=on", | |||
"test:unit": "vitest run --mode development --exclude **/jest-component-unit-tests/*", | |||
"test:unit:components": "jest -c jest-component-unit-tests/jest.config.ts --rootDir jest-component-unit-tests/", |
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.
jest
is blown away. npm run test:unit
will cover the new component files. Vitest does all the heavy lifting!
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.
Thank you!
I was definitely concerned about having two runners willy nilly.
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.
Awesome! Please delete npm run test:unit:components
from the Makefile
as well.
@@ -127,8 +127,28 @@ export const ModelingMachineProvider = ({ | |||
app: { allowOrbitInSketchMode }, | |||
modeling: { defaultUnit, cameraProjection, cameraOrbit }, | |||
} = useSettings() | |||
const { context } = useFileContext() | |||
const { file } = useLoaderData() as IndexLoaderData | |||
const loaderData = useLoaderData() as IndexLoaderData |
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.
The modelingmachine needs to point to the Project
which was in filemachine provider but now it is a function of systemIOMachine and your IndexLoaderData
.
I only updated this code to have reference to the project for TTC workflows.
@@ -0,0 +1,175 @@ | |||
import React, { useEffect, useMemo } from 'react' |
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.
This was the fileMachineProvider.tsx
but now it is called ModelingPageProvider
because it has some initialization for the modeling page. No more file machine related stuff.
This will most likely get removed in the future when more command logic gets refactored.
@@ -0,0 +1,347 @@ | |||
// @ts-nocheck |
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've moved Lee's file and renamed it for vitest and added ts-nocheck
because the tsc checker in the jest codebase was not the same as our monorepo one.
This is a future todo. File still passes its tests.
@@ -24,6 +24,11 @@ import { | |||
} from '@src/machines/systemIO/utils' | |||
import { fromPromise } from 'xstate' | |||
import type { AppMachineContext } from '@src/lib/types' | |||
import { |
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.
Updated SystemIOMachine to have the same workflows as the FileMachine which is now deleted. These are required for generic application workflows as well as the project explorer.
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.
Really nice work.
I'm curious about UseFileSystemWatcher not being in yet, what is the consequence of this? user's can have ZDS and vscode open at the same time?
Also "status dots don't do anything in the file tree yet" should we just hid these until they do something?
I'm also curious if you've tried opening a monsterous directory? I can imagine perf issues that are outside of the file io stuff, just even just needing a virtual list to make react happy. Not saying those kinds of optimizations need to be in or anything, just think it's worth sanity checking and having some idea of where the limit might be before we merge.
Users won't be able to have them open at the same time and sync. I am worried about the implementation of the watcher.
No? They can click the file in the row to see that the row has an error. There isn't any extra information in the dot.
Yes there are performance issues. We have them currently in the production application. I am working through the performance issues right now. There are many problems. I'll profile them and report my findings and fixes. |
@nadr0 Sorry haven't caught up to the rest of the PR yet. Does that mean we'd also loose the ability to have two ZDS windows open side by side (the Open in new window introduced in #6379)? That was a workaround for v1 to be able to edit a part while viewing the assembly at the same time. |
Yes, useFileSystemWatcher is disabled no live update from external sources. Having two applications opened "updates" the other one because the other application is writing to disk and then the other application sees that an external file is edited. |
closes #6878
tl;dr for review
Locator
changesKnown bugs
main
the context menu does not recompute based on the row you select, it will keep the selected row then render at a newx,y,
position.main
closing the pane will unmount the file tree and lose its local stateIssue
Implementation
vitest
it works...npm run test:unit:components
is deleted because thenpm run test:unit
covers this case with vitest since jest .tsx testing is in vitest nowFeatures
Two missing features in new file tree
UseFileSystemWatcher (disk watch) is removed in this version will be back.
Clone is deleted as well since it is implemented wrong and I want cut/copy/paste.
Everything else should be implemented.
Arrow keys
Enter
New action header bar
Sorted placeholders for new file or new folder
Rename
3 States for Rows
Linear layout of the DOM instead of recursive DOM structure
Aria support for div soup/linear layout (1 bug listed above)
Folder/files will have a pink dot if there is an LSP error (all parent folders to nest down to the problematic file)
Oddly specific subtle features
Testing
Locators
in playwright and the fucking tests pass!! hell yeah! I didn't rewrite any logicScreenshot
Future work