Skip to content
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

react updates #885

Merged
merged 11 commits into from
Jan 10, 2025
Merged

react updates #885

merged 11 commits into from
Jan 10, 2025

Conversation

binaryberserker
Copy link
Contributor

No description provided.

@binaryberserker binaryberserker marked this pull request as draft January 3, 2025 22:36
@binaryberserker binaryberserker self-assigned this Jan 3, 2025
@binaryberserker binaryberserker changed the title react module 2 updates react updates Jan 8, 2025
@binaryberserker binaryberserker marked this pull request as ready for review January 9, 2025 17:27
@binaryberserker binaryberserker enabled auto-merge (squash) January 9, 2025 17:28
@@ -191,7 +198,7 @@ Remove these generated files that we won’t need. Some projects do need them, b
- `src/assets`
- `src/App.css`
- `tsconfig.node.json`
- `.eslintrc.cjs`
- `.eslintrc.cjs` or `eslint.config.js`

✏️ Uninstall unneeded packages and Install our eslint config and prettier:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the future, we should split this into two commands so it’s more clear and easier to copy/paste.

@@ -165,9 +165,9 @@ Before we begin requesting data from our API, we need to install the `place-my-o
npm install place-my-order-api@1
```

✏️ Next add an API script to your `package.json`
✏️ Next add an API script to your `package.json`. NOTE: if on windows, change `/` to `\\`.
Copy link
Member

@chasenlehara chasenlehara Jan 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two nits:

  • Next add -> Next, add
  • windows -> Windows

And actually, do we need the ./node_modules/.bin/ in the command? Then we wouldn’t need the extra instructions for Windows.

Copy link
Member

@chasenlehara chasenlehara left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I went through the first couple sections to double check things and found some nits, but all of these can be fixed the next time we touch the code.

I’m going to merge this in as-is.

@@ -1,7 +1,6 @@
import { useState } from "react"
import reactLogo from "./assets/react.svg"
import viteLogo from "./assets/vite.svg"
import "./App.css"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The -solution folders are meant to be the state of the app after completing the exercise, so at this point, this import still exists and should be left in.

@@ -1,7 +1,6 @@
import { useState } from "react"
import reactLogo from "./assets/react.svg"
import viteLogo from "./assets/vite.svg"
import "./App.css"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here, this should be left in.

@@ -1,5 +1,3 @@
import "./App.css"

function App() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have a disparity between the previous solution:

And this function App() line. I wonder which we’re meant to have? cc @rjspencer

@@ -4,6 +4,7 @@
"version": "0.0.0",
"type": "module",
"scripts": {
"api":"./node_modules/.bin/place-my-order-api -p 7070",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"api":" -> "api": " (here and everywhere else)

@binaryberserker binaryberserker merged commit f242b8e into main Jan 10, 2025
1 check passed
@binaryberserker binaryberserker deleted the dbrandon/react-module-2-updates branch January 10, 2025 20:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants