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.

Sign up for GitHub

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

Jump to bottom

Folder Refactor #232

Merged
merged 7 commits into from
Dec 29, 2020
Merged

Folder Refactor #232

merged 7 commits into from
Dec 29, 2020

Conversation

hahnbeelee
Copy link
Contributor

@hahnbeelee hahnbeelee commented Dec 14, 2020 •
edited
Loading

Summary

  • Added a containers folder where the main "pages" lie (Login, 404, Dashboard, Settings (?))
  • Organized components into folders
    • All components related to Requirements/SemesterView/BottomBar in one folder
    • Components that are re-used in multiple places are kept at the highest level Components folder ( e.g. Course.vue and DropdownArrow.vue)

Test Plan

  • Make sure npm run serve works
  • Look through the file structure and see if it makes sense
    • specifically, see if there are some components in sub-folders that should be at the highest level.

Notes

We are holding off on merging this till winter break to reduce merge conflicts while people are finishing up their final PRs for the semester.

@hahnbeelee hahnbeelee requested a review from a team December 14, 2020 03:32
@dti-github-bot
Copy link
Member

dti-github-bot commented Dec 14, 2020 •
edited
Loading

[diff-counting] Significant lines: 83.

@dti-github-bot
Copy link
Member

[deployment-bot] Deployed to https://cornell-dti.github.io/course-plan/232/index.html

hahnbeelee
hahnbeelee commented Dec 14, 2020
View reviewed changes
src/components/Navigation.vue
@@ -1,9 +0,0 @@
<template>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this didn't seem like it was being used anywhere and my GitLens extension was telling me that it was added on first commit so I'm assuming this is outdated lol

This comment was marked as resolved.

Sign in to view

@hahnbeelee hahnbeelee linked an issue Dec 14, 2020 that may be closed by this pull request
Consider moving views to a separate folder under src #13
Closed
SamChou19815
SamChou19815 approved these changes Dec 14, 2020
View reviewed changes
Copy link
Contributor

@SamChou19815 SamChou19815 left a comment

Choose a reason for hiding this comment

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

Actually looks good to go. git can detect changed files so merge conflicts won't be too bad.

@hahnbeelee
Copy link
Contributor Author

@willespencer thoughts on just merging this rn?

willespencer
willespencer requested changes Dec 15, 2020
View reviewed changes
Copy link
Member

@willespencer willespencer left a comment

Choose a reason for hiding this comment

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

Thanks for doing this Hahnbee! I'm trusting Sam on this one so I do think we can merge it in, just make sure you mention that this happened in cp-dev after this gets finished. I also have a couple suggested changes haha.

  • Can you move the 4 onboarding components and 1 scss file to their own separate folder as well?
  • Can you delete Settings.vue altogether instead of moving it? It is a remnant from our starter code as well. You probably have to remove it from the Router too.

src/components/Modals/NewCourse/BinaryButton.vue Outdated Show resolved Hide resolved
src/components/Navigation.vue
@@ -1,9 +0,0 @@
<template>

This comment was marked as resolved.

Sign in to view

@hahnbeelee hahnbeelee changed the title Folder Refactor [DO NOT MERGE TILL WINTER BREAK] Folder Refactor Dec 15, 2020
@hahnbeelee
Copy link
Contributor Author

Thanks for doing this Hahnbee! I'm trusting Sam on this one so I do think we can merge it in, just make sure you mention that this happened in cp-dev after this gets finished. I also have a couple suggested changes haha.

  • Can you move the 4 onboarding components and 1 scss file to their own separate folder as well?
  • Can you delete Settings.vue altogether instead of moving it? It is a remnant from our starter code as well. You probably have to remove it from the Router too.

Done!

willespencer
willespencer approved these changes Dec 16, 2020
View reviewed changes
Copy link
Member

@willespencer willespencer left a comment

Choose a reason for hiding this comment

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

Looks good! Talked over slack but let's just wait till #159 is merged in to be extra safe!

@SamChou19815 SamChou19815 merged commit 7ca5636 into master Dec 29, 2020
@SamChou19815 SamChou19815 deleted the folder-refactor branch December 29, 2020 02:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SamChou19815 SamChou19815 SamChou19815 approved these changes

@willespencer willespencer willespencer approved these changes

Assignees
No one assigned
Labels
large
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Consider moving views to a separate folder under src
4 participants
@hahnbeelee @dti-github-bot @SamChou19815 @willespencer