-
Notifications
You must be signed in to change notification settings - Fork 18
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
Update README for ui development. #146
Update README for ui development. #146
Conversation
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.
LGTM for the second part. Left a couple comments in the first part.
src/ui/README.md
Outdated
`components/` that is published to npm, and an application server in `app/` | ||
that consumes `components/`. | ||
`common/` that is published to npm as a package called `@aqueducthq/common`, and an application server in `app/` | ||
that consumes `@aqueducthq/common`. | ||
|
||
If you're actively developing the UI, you will need to copy the `.env.local` file |
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.
@vsreekanti @agiron123 is the .env.local
file still relevant? I don't think we have ~/.aqueduct/ui/app
folde anymore right? The directory just contains a few "compiled" UI files.
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.
No -- in development mode, we still need a .env
file. But yes, we don't have ~/.aqueduct/ui
.
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.
@vsreekanti @cw75 just want to confirm that all we need in env.local is the SERVER_ADDRESS field, correct?
SERVER_ADDRESS=http://localhost:8080
`SERVER_ADDRESS=3.142.105.48:8080`). Then, you can start the Next.js server in dev | ||
mode by running `make dev`. Note that this will symlink your local version of | ||
the `components/` directory to pick up any changes you might have made. | ||
`SERVER_ADDRESS=3.142.105.48:8080`). |
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.
Stray close parens.
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 isn't about exposing the UI at a public IP. This is about whether the server is running on a different machine than the UI.
`SERVER_ADDRESS=3.142.105.48:8080`). Then, you can start the Next.js server in dev | ||
mode by running `make dev`. Note that this will symlink your local version of | ||
the `components/` directory to pick up any changes you might have made. | ||
`SERVER_ADDRESS=3.142.105.48:8080`). |
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 isn't about exposing the UI at a public IP. This is about whether the server is running on a different machine than the UI.
Describe your changes and why you are making these changes
Updates README for building and making changes to ui codebase.
Related issue number (if any)
https://linear.app/aqueducthq/issue/ENG-1280/cleanup-outdated-ui-documentation-in-our-repo
https://linear.app/aqueducthq/issue/ENG-1268/update-ui-readme-file
Checklist before requesting a review