-
Notifications
You must be signed in to change notification settings - Fork 16
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
[Issue 2498] update frontend readme for current file structure #2247
[Issue 2498] update frontend readme for current file structure #2247
Conversation
│ └── styles # Sass & design system settings | ||
├── stories # Storybook pages | ||
└── tests | ||
``` | ||
|
||
## 💻 Development | ||
|
||
[Next.js](https://nextjs.org/docs) provides the React framework for building the web application. Pages are defined in the `pages/` directory. Pages are automatically routed based on the file name. For example, `pages/index.tsx` is the home page. | ||
|
||
Files in the `pages/api` are treated as [API routes](https://nextjs.org/docs/api-routes/introduction). An example can be accessed at [localhost:3000/api/hello](http://localhost:3000/api/hello) when running locally. |
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.
did we used to support this at the /api route? Is that no longer a thing or do we need to implement a route.ts file to handle API requests into Next? https://nextjs.org/docs/app/api-reference/file-conventions/route
Based on the discussion about the API today, it seems like would likely want to avoid this and instead direct traffic to the API
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 do see a subscribe.ts
under pages/api, and it looks like it is called from 'NewsletterForm.tsx', whether or not that means we need a routes.ts, not sure.
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.
yeah, that's true, but I think we're in the process of deprecating that, so I was thinking we should remove it from the README at this point. Does that sound right @acouch ?
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.
did we used to support this at the /api route?
Yes, that is how the pages router works. With the move to the app router what you removed doesn't make sense.
Coverage report for
|
St.❔ |
Category | Percentage | Covered / Total |
---|---|---|---|
🟢 | Statements | 88.47% | 928/1049 |
🟡 | Branches | 73.49% | 244/332 |
🟢 | Functions | 84.11% | 180/214 |
🟢 | Lines | 88.75% | 868/978 |
Test suite run success
188 tests passing in 59 suites.
Report generated by 🧪jest coverage report action from 44d6beb
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.
@doug-s-nava Doc update looks great. The title could be updated for the [Issue #N]
convention. 🚀
Summary
Issue #2498
Time to review: 5 mins
Changes proposed
frontend README updated for current file structure
Context for reviewers
Test Steps
Additional information