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

[#2001] Extract c-authorship-file component from views/c-authorship #2096

Merged
merged 9 commits into from
Feb 6, 2024

Conversation

sopa301
Copy link
Contributor

@sopa301 sopa301 commented Jan 23, 2024

Part of #2001

Proposed commit message

Many of the frontend files are difficult to navigate through due to the
large file sizes. As we add more features to the frontend, it is
getting harder to maintain. This is also very unfriendly towards new
contributors. 

Let us break down frontend files in a logical manner, starting with
extracting c-authorship-file from views/c-authorship.

This would provide a starting point towards further refactoring of the
frontend.

Other information

There was a bug in dynamicMixinTooltip.ts where if a component tried to access the methods that involve looking for specific elements while those elements are not in a v-for loop, the result returned by this.$ref[elementName] would not be an array, causing the code to crash. Credits to @jonasongg for discovering this bug and providing the solution.

Also, there seems to be some scss code labelled as segment in the c-authorship-file which should be in c-segment instead. Perhaps it should be shifted there, maybe in another PR.

@sopa301 sopa301 marked this pull request as ready for review January 23, 2024 15:35
@sopa301 sopa301 requested a review from a team January 23, 2024 15:35
@ckcherry23
Copy link
Member

ckcherry23 commented Jan 25, 2024

Hi @sopa301, we appreciate your efforts in trying this out! While this approach does break down the huge file into smaller parts, it creates multiple files that are highly interrelated instead of multiple loosely coupled components with good separation of concerns.

I think it is better if we do our modularization by continuing to follow the Single-File Component pattern of Vue. Let us try to extract the various smaller components within the c-authorship.vue to create smaller SFCs instead. For example, we could extract a c-file.vue for each file displayed and a c-authorship-picker.vue for the various dropdowns and checkboxes on top.

@sopa301
Copy link
Contributor Author

sopa301 commented Jan 25, 2024

Thanks for the feedback! I'll look into extracting out a c-file first.

@ckcherry23 ckcherry23 removed the request for review from a team January 26, 2024 11:44
@sopa301 sopa301 marked this pull request as ready for review January 28, 2024 12:55
@sopa301 sopa301 requested a review from a team January 28, 2024 12:55
Copy link
Contributor

@jonasongg jonasongg left a comment

Choose a reason for hiding this comment

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

Other than the brackets thing, LGTM!

* if the ref is on a v-for loop, else it will be a single HTMLElement.
*/
getElementByRef(refName: string): HTMLElement {
return Array.isArray((this.$refs[refName]))
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor nit, but there brackets here are doubled

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for catching that! I fixed it.

@jonasongg jonasongg requested a review from a team January 28, 2024 13:36
Copy link
Member

@ckcherry23 ckcherry23 left a comment

Choose a reason for hiding this comment

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

LGTM!

I was wondering if we could move some code of processFiles() into child components so that the individual file processing that is not required by the parent c-authorship.vue component can be moved into c-authorship-file.vue. If possible (and helpful - more code explainability, chances of better performance), we can look into this in a future PR.

@ckcherry23 ckcherry23 requested a review from a team January 29, 2024 06:46
@ckcherry23
Copy link
Member

@sopa301 Can you also update the PR title to refer to the c-authorship-file component?

@sopa301 sopa301 changed the title [#2001] Split views/c-authorship into smaller components [#2001] Extract c-authorship-file component from views/c-authorship Jan 29, 2024
@ckcherry23
Copy link
Member

@vvidday Do give us your thoughts on this

I was wondering if we could move some code of processFiles() into child components so that the individual file processing that is not required by the parent c-authorship.vue component can be moved into c-authorship-file.vue. If possible (and helpful - more code explainability, chances of better performance), we can look into this in a future PR.

Copy link
Contributor

@vvidday vvidday left a comment

Choose a reason for hiding this comment

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

Great work on this PR @sopa301! This is a great step towards our goal of breaking down the giant components. Just have some very minor questions.

Regarding the CSS changes, I agree that can be done in a separate PR.

frontend/src/components/c-authorship-file.vue Outdated Show resolved Hide resolved
frontend/src/views/c-authorship.vue Outdated Show resolved Hide resolved
@vvidday
Copy link
Contributor

vvidday commented Feb 4, 2024

@vvidday Do give us your thoughts on this

I was wondering if we could move some code of processFiles() into child components so that the individual file processing that is not required by the parent c-authorship.vue component can be moved into c-authorship-file.vue. If possible (and helpful - more code explainability, chances of better performance), we can look into this in a future PR.

Hmm the only thing I'm concerned regarding this is that currently the authorship component is updating the entire AuthorshipFile[] array into the store (this.$store.commit('updateTabAuthorshipFiles', res);) after processing. If the processing is done in the child components instead, we would have to be very careful about how we are updating the store and whether any changes to the store update logic would affect any other areas.

@vvidday vvidday self-requested a review February 4, 2024 13:28
Copy link
Contributor

@vvidday vvidday left a comment

Choose a reason for hiding this comment

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

LGTM!

@ckcherry23 ckcherry23 merged commit b52771b into reposense:master Feb 6, 2024
8 checks passed
Copy link
Contributor

github-actions bot commented Feb 6, 2024

The following links are for previewing this pull request:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants