refactor(ui): flatten ui/src/app dir
#13815
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
agilgur5
commented
Oct 25, 2024
There was a problem hiding this comment.
the {} -> any change made this a full delete/add instead of a rename
There was a problem hiding this comment.
not sure why git didn't detect this as a rename 🤔 possibly because of the existing symlink in ui/tsconfig.json? it's not letting me comment on specific lines in that file either
EDIT: yea it seems to be because of the prior symlink
There was a problem hiding this comment.
outDir here and output.path in webpack.config.js need a bit of carefulness since they're used in the Dockerfile and Makefile
added 3 commits
October 25, 2024 13:28
- move `ui/src/models` to `ui/src/shared/models` - and flatten the rest - also fix name of `workflow-operation-map.tsx` to `.ts` as it doesn't have JSX/TSX - also move `webpack.config.js` out of `ui/src/app` and to `ui/` - same with `tsconfig.json` - remove symlinks and config that was previously necessary due to having config in the subdirectory Signed-off-by: Anton Gilgur <[email protected]>
- these all failed to typecheck due to using `{}` as a type -- use `any` instead
Signed-off-by: Anton Gilgur <[email protected]>
Signed-off-by: Anton Gilgur <[email protected]>
agilgur5
force-pushed
the
refactor-ui-flatten-app-dir
branch
from
7cc27cc to
b4735a6
Compare
Signed-off-by: Anton Gilgur <[email protected]>
tczhao
approved these changes
Oct 28, 2024
|
Yes I mentioned that under "Modifications" |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #12539 "Future Work" part 2
Motivation
Remove unnecessary nesting, same as #12539
Modifications
move
ui/src/modelstoui/src/shared/modelsthen flatten
ui/src/appto justui/src/also fix name of
workflow-operation-map.tsxto.tsas it doesn't have JSX/TSX (fixes feat: Unified workflows list UI and API #11121 (comment))also move
webpack.config.jsout ofui/src/appand toui/tsconfig.jsonVerification
make start UI=trueloads the UI fine and dandy and it works as beforeyarn lintandyarn buildetc all passmake server/static/files.go STATIC_FILES=trueworks/passesls dist/applooks about the same before/after:before:
after:
Notes to Reviewers
Use the sidebar in GH to see the files that weren't just renamed, which is only a small handful and mostly the last few config files
This will merge conflict with some of my other open PRs that delete or change imports; can update depending on what is merged first