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

Restructure front end #647

Merged
merged 4 commits into from
Feb 5, 2020
Merged

Conversation

miggs125
Copy link
Contributor

@miggs125 miggs125 commented Feb 4, 2020

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.

  • Bugfix: Change which fixes an issue
  • New Feature: Change which adds functionality
  • Documentation Update: Change which improves documentation
  • UI: Change which improves UI

Description

Checklist

  • Quality: This PR builds and passes our npm test and works locally
  • Tests: This PR includes thorough tests or an explanation of why it does not
  • Screenshots: This PR includes screenshots or GIFs of the changes made or an explanation of why it does not (if applicable)
  • Documentation: This PR includes updated/added documentation to user exposed functionality or configuration variables are added/changed or an explanation of why it does not(if applicable)

@vercel
Copy link

vercel bot commented Feb 4, 2020

This pull request is being automatically deployed with ZEIT Now (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://zeit.co/humphd/telescope/ku98rctan
✅ Preview: https://telescope-git-fork-miggs125-restructurefrontend.humphd.now.sh

humphd
humphd previously approved these changes Feb 4, 2020
Copy link
Contributor

@humphd humphd left a 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).

@miggs125
Copy link
Contributor Author

miggs125 commented Feb 4, 2020

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?

@humphd
Copy link
Contributor

humphd commented Feb 4, 2020

@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 .jsx and .js into a single .js file.

cindyorangis
cindyorangis previously approved these changes Feb 4, 2020
Copy link
Contributor

@cindyorangis cindyorangis left a 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;
Copy link

@wlto wlto Feb 4, 2020

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 :)

@miggs125 miggs125 merged commit 4260501 into Seneca-CDOT:master Feb 5, 2020
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.

5 participants