- Notifications
You must be signed in to change notification settings - Fork928
chore: improve content of frontend contributing guide#14619
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
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.
I like the changes overall! Just had a few suggestions
Uh oh!
There was an error while loading.Please reload this page.
| - **components** - Reusable UI components without Coder specific business | ||
| logic |
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.
@jaaydenh@aslilac@BrunoQuaresma This was actually something I was going to ask about for this week's frontend variety, but do you think that going forward, we should enforce a "no data fetching in the components directory" rule?
We currently have some for the filter options and the autocomplete components, but my hope would be either:
- Move these to modules
- Decouple the layout concerns for the components from the fetching, and move the fetching somewhere else
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 filter options logic technically isn't hitting an API endpoint itself, but it's still doing weird stuff like putting JSX directly inside the query cache
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 filter component is a bad example 😓, so we shouldn't consider it a best practice. However, I do think it makes sense not to have fetching logic inside 'shareable' 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.
@Parkreiner Do you think there is anything specific to call out in the docs about this right now?
Uh oh!
There was an error while loading.Please reload this page.
docs/contributing/frontend.md Outdated
| should always try to use the base components that are provided by the library or | ||
| from the codebase. It's recommended that you always do a quick search before | ||
| creating a custom primitive component like dialogs, popovers, buttons, etc. | ||
| Do not assume existing components and modules have been categorized correctly. |
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.
| Do not assume existingcomponentsand modules have been categorized correctly. | |
| Our codebase has some legacycomponentsthat are being updated to follow these new conventions, but all new components should follow these guidelines. |
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.
Feel like the current wording is throwing the team under the bus a bit
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.
@Parkreiner hehe, you are totally correct, I was just trying to be really clear about the current state of things to avoid confusion. But I agree your wording sounds much better.
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 wanted to create another PR eventually to move some components to modules.
| In both cases, you can access the dashboard on `http://localhost:8080`. If you | ||
| are running the `./scripts/develop.sh` you can log in using the default | ||
| credentials: `admin@coder.com` and `SomeSecurePassword!`. | ||
| > **Default Credentials:** `admin@coder.com` and `SomeSecurePassword!`. |
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.
nit: this is not quite the GHFM "callout" that@Parkreiner was suggesting, this is just a block quote, which is also kind of a misrepresentation
Uh oh!
There was an error while loading.Please reload this page.
| making API requests. The API functions are centralized in `site/src/api/api.ts`. | ||
| Auto-generated TypeScript types derived from our Go server are located in | ||
| `site/src/api/typesGenerated.ts`. |
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.
although it looks like we're inconsistent about it...sigh
Uh oh!
There was an error while loading.Please reload this page.
40688e4 intomainUh oh!
There was an error while loading.Please reload this page.
The goal of this update are:
This refactor does not address the testing section
In the future, more information about theming should be included in the guide.