Reimplemented using modern react, added daily specials #8
Conversation
…into modern-react
…into modern-react
…into modern-react
…into modern-react
…into modern-react
…into modern-react
public/manifest.json
Outdated
| @@ -0,0 +1,25 @@ | |||
| { | |||
| "short_name": "React App", | |||
There was a problem hiding this comment.
- Looks like you checked in the React logos (public/logo*)
Also, you should update this to be production values, this manifest gets used by Android and friends.
src/ components/EateryCard.js
Outdated
| } from "@mui/material"; | ||
| import ExpandMoreIcon from "@mui/icons-material/ExpandMore"; | ||
|
|
||
| const StyledCard = styled(Card)({ |
There was a problem hiding this comment.
I like the use of styled components, but if you're going to use them then your CSS is now code and you probably want to use some constants somewhere to get rid of all the duplication in the colors, font families, etc.
src/ components/EateryCard.js
Outdated
| statusMsg, | ||
| todaysSoups, | ||
| }) { | ||
| const [modalOpen, setModalOpen] = useState(false); |
There was a problem hiding this comment.
Name boolean values with is or another such word (has, should, etc) i.e. isModalOpen, setIsModalOpen
src/ components/EateryCard.js
Outdated
| isOpen, | ||
| statusMsg, | ||
| todaysSoups, | ||
| }) { |
There was a problem hiding this comment.
If you're modernizing everything, I'd suggest using Typescript.
| statusMsg={statusMsg} | ||
| todaysSpecials={todaysSpecials || []} | ||
| todaysSoups={todaysSoups || []} | ||
| key={index} |
There was a problem hiding this comment.
nit: setting the key to the index defeats the purpose of the key. Just use the name since that should be unique, right?
| @@ -0,0 +1,13 @@ | |||
| body { | |||
| margin: 0; | |||
| font-family: -apple-system, BlinkMacSystemFont, 'Segoe UI', 'Roboto', 'Oxygen', | |||
There was a problem hiding this comment.
If you're inlining fonts everywhere, then don't have CSS specifying this. Or pick one or the other.
| // If you want to start measuring performance in your app, pass a function | ||
| // to log results (for example: reportWebVitals(console.log)) | ||
| // or send to an analytics endpoint. Learn more: https://bit.ly/CRA-vitals | ||
| reportWebVitals(); |
There was a problem hiding this comment.
Are you actually using web vitals? No worries if not, everyone just forgets that this file is here haha.
| } else if (hours >= 17 && hours < 24) { | ||
| message = evening[Math.floor(Math.random() * evening.length)]; | ||
| } | ||
| const getGreeting = () => { |
There was a problem hiding this comment.
Nit: just export default function getGreeting.
| @@ -1,5 +1,4 @@ | |||
| var hours = new Date().getHours(); | |||
There was a problem hiding this comment.
nit: Luke-warm take: Util directories are useless, they do not actually add anything to helping find the file or make it tidier :)
nit 2: if you have a single top-export, call the file the same thing (thus: getGreeting.js)
| async function queryLocations() { | ||
| try { | ||
| // Query locations | ||
| const { data } = await axios.get(BASE_URL); |
There was a problem hiding this comment.
Nit: Now that fetch has gotten so good, I don't see the value in loading a library to do what is now a standard utility.
Demo available at https://dining.scottylabs.org/