-
Notifications
You must be signed in to change notification settings - Fork 11
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
Cleanup Vue components #279
Conversation
[diff-counting] Significant lines: 467. |
Visit the preview URL for this PR (updated for commit 98c77de): https://cornelldti-courseplan-dev--pr279-vue-component-cleanu-wywls67f.web.app (expires Fri, 19 Feb 2021 05:57:28 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 |
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.
Thanks for this great improvement in the codebase Sam! I left some comments but once those are addressed, the PR should be good to go!
import store from '@/store'; | ||
import store from '../store'; |
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.
Is it better to use ../
here instead of @/
? I thought @
referred to src
.
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 is used to make the ts project for requirement generator happy
<bottombar | ||
<bottom-bar |
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.
Without declaring the name of the component as in the old style (Vue.component('bottombar', BottomBar);
), how do we know that bottom-bar refers to the component BottomBar? Just curious!
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 some standard case transform vue performs. CamelCase to lower-case-with-dash
Vue.component('semesterview', SemesterView); | ||
Vue.component('requirements', Requirements); |
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.
Are we waiting on handling these until after the migration to the global data store?
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.
Yes
contentClass="content-course" | ||
content-class="content-course" |
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.
Isn't the required prop name contentClass
?
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.
Also some case transformation on props
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.
LGTM! Thank you so much Sam.
## Summary This diff continues the work of #279. It refactors the IncompleteSubReqCourse component so that the prop no longer relies on `CrseInfo` and `SubReqCourseSlot`, so that we can cleanup the last `Vue.component(...)` call that doesn't work well with Vuetr. I also moved the type of `SubReqCourseSlot` from `src/requirement/types` into `SubRequirement.vue`, since only that component and `Requirement.vue` are using it. ## Test Plan See all courses and incomplete subreq courses still load
Summary
Cleanup vue components by
components: {...}
, instead ofVue.component('name', Component)
. This gives better type-checking and autocompletion.Unfortunately, the Vuetr plugin has a bug with imported types in props. Therefore, I have to make all the common types global.
Test Plan
Move around the app and see no warnings in console.