-
Notifications
You must be signed in to change notification settings - Fork 188
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
Restructure front end #647
Conversation
This pull request is being automatically deployed with ZEIT Now (learn more). 🔍 Inspect: https://zeit.co/humphd/telescope/ku98rctan |
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.
Code-wise this looks fine to me. The only thing I'd suggest you consider is that you're somewhat inconsistent with your naming of .js
vs. .jsx
vs. .css
files for components. For example:
textArea.css
vs. TextArea.jsx
and then
ListItem.jsx
and ListItem.css
I'd probaby keep everyting the same (i.e,. same as name of component).
@humphd I wasn't sure what the standard is. Are styles usually lower case but the same name? |
@miggs125 usually I've seen it done with the same name for all files, e.g. https://create-react-app.dev/docs/adding-a-stylesheet. You could also consider flattening your |
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.
It's weird being able to see our frontend on mobile (I'm at my bus stop looking at this PR) now. It's a good weird. Thanks for getting rid of the SASS stuff.
@@ -2,10 +2,10 @@ | |||
width: 80vw; | |||
background: rgba(255, 255, 255, 0.7); | |||
margin: 3rem auto 0; | |||
font-size: $medium-font-size; | |||
font-size: 1.7rem; | |||
padding: rem 10rem; |
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 think we're missing the number of padding for top and bottom here. Other than this the code looks good to me :)
bf237b3
to
985dbcb
Compare
Issue This PR Addresses
#583 #584
Type of Change
This PR restructures the frontend to contain component logic and styling under the same directory. It also removes SASS as a dependency.
Description
Checklist