🚀 feat(inspect): Add project management #2977
Conversation
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
MarkRedeman
left a comment
MarkRedeman
left a comment
There was a problem hiding this comment.
I have some minor comments but they are not intended as blockers (some stuff depends on how we want to make project management be consistent across all the apps).
| onPress={addProject} | ||
| > | ||
| <AddCircle /> | ||
| <Text marginX='size-50'>Add project</Text> |
There was a problem hiding this comment.
Later on I'd expect this to open a small dialog where the user can fill in the project name.
| }, | ||
| }); | ||
|
|
||
| onSetProjectInEdition(newProjectId); |
There was a problem hiding this comment.
I'd expect the user to be redirected to the new project here.
| "baseUrl": ".", | ||
| "paths": { | ||
| "@geti-inspect/icons": ["./src/assets/icons"] | ||
| "@geti-inspect/icons": ["./src/assets/icons"], |
There was a problem hiding this comment.
np: personally I'm not a huge fan of having absolute imports for these because it makes it harder to copy over code from inspect to the other applications and vice versa.
As an example: last time when I copied the project list panel component from prompt to action I had to manually update the imports because they were using @geti-prompt. If instead they had used relative imports then it wouldn't require any changes.
This isn't a huge deal but might be worth considering. Personally I don't see a huge advantage of absolute imports as long as we keep our folder structure tidy.
| <Text>{project.name}</Text> | ||
| </Flex> | ||
| )} | ||
| {/*<ProjectActions onAction={handleAction} />*/} |
There was a problem hiding this comment.
Perhaps we can omit all the project actions' code for now and copy them in later?
dwesolow
force-pushed
the
dwesolow/project-management
branch
from
e027cef to
22fb224
Compare
| useEffect(() => { | ||
| textFieldRef.current?.select(); | ||
| }, []); |
There was a problem hiding this comment.
I think alternatively you could use autofocus on the TextField.
| const projectToUpdate = projects.find((project) => project.id === projectId); | ||
| if (projectToUpdate?.name === newName || isEmpty(newName.trim())) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Nothing seems to be happening here, maybe add a TODO, or an alert to remind ourselves?
| path: paths.project.pattern, | ||
| element: ( | ||
| <Suspense fallback={<Loading mode='fullscreen' />}> | ||
| <Suspense fallback={<IntelBrandedLoading />}> |
There was a problem hiding this comment.
note: on thing I briefly mentioned to Joao is that we should add a variant={"spinner" | "intel-blocks"} option to our <Loading> component so that we can easily use both.
* chore: Add project route * feat: Add project management * revert ui lock change * chore: Remove not needed code for project management
* chore: Add project route * feat: Add project management * revert ui lock change * chore: Remove not needed code for project management
* chore: Add project route * feat: Add project management * revert ui lock change * chore: Remove not needed code for project management
4 participants
📝 Description
This PR introduces project management that displays list of the projects and current project in the navbar.
On Tuesday we will discuss adding a projects list when opening an app without project id included in the URL. After that meeting I'll add all the necessary changes but this PR is fine to go at the moment.
✨ Changes
Select what type of change your PR is:
✅ Checklist
Before you submit your pull request, please make sure you have completed the following steps:
For more information about code review checklists, see the Code Review Checklist.