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

major refactor to our codebase for long term development #9

Merged
merged 3 commits into from
Jun 9, 2020

Conversation

yqj2k
Copy link
Contributor

@yqj2k yqj2k commented Jun 8, 2020

No description provided.

@yqj2k yqj2k requested review from baconandchips, DSep and beetai June 8, 2020 04:36
Copy link
Member

@DSep DSep 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!
Just one thing: does it make sense to contain all the functionality like browse/searching and course viewing within a profile directory? I feel like those two don't really fall into the category, but I do like the idea of separating. So maybe they can be in their own dirs?

@yqj2k
Copy link
Contributor Author

yqj2k commented Jun 8, 2020

Maybe I should rename the profile directory... I do like the idea of keeping them together; I feel like these two are workflows/actions that the user would do.

@beetai
Copy link
Contributor

beetai commented Jun 8, 2020

I don't see a problem and all looks good to me, though I agree that it's worth discussing whether course search should be within profile. I was thinking maybe you'd have the profile be one folder and course search be another? Once this is sorted out I'm ready to approve this to merge.

@DSep
Copy link
Member

DSep commented Jun 9, 2020

Yeah, I feel your (Sam) page (to be renamed Home) would make the most sense under "Profile" and Guanting and I's pages would be more logical under the main directory. But also remember that these are Pages, more general than the Components within them, and much fewer of them to try to group by function. Perhaps it makes sense to not really be generating too many subdirs for the Pages?

@yqj2k
Copy link
Contributor Author

yqj2k commented Jun 9, 2020

I agree with you that Browse/Search is too general to be in another profile subdirectory, but I think they are components within a user's actions. I don't think they would be pages because our page should only refresh the user's actions (browsing, or course searching) all the while a navbar and other things on the page will remain the same.

So, why don't we put Browse/Search within the component folder for now, and for future iterations, if they do become standalone general components, why don't we create folders for themselves?

@DSep DSep self-requested a review June 9, 2020 08:09
Copy link
Member

@DSep DSep left a comment

Choose a reason for hiding this comment

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

LGTM
@baconandchips 's VisualizeCourse pieces will need to be moved to Components later.

And all of our pages will be need to be modularized as much as reasonably possible later.

@yqj2k yqj2k merged commit 29698ef into develop Jun 9, 2020
@baconandchips
Copy link
Contributor

Looks good with me as well! I agree on the component move; will be doing that when I get off work. I think having Profile might be a bit general for the components as of now; maybe we can start off with having page components in folders for their respective pages under "Components" folder, and as we develop commonly-used components we can perhaps move them to the main "Components" folder, or we develop them directly in the "Components" folder. We can definitely discuss more in the meeting to agree on a standard

@beetai beetai deleted the sam-refactor branch July 11, 2020 08:03
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.

4 participants