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-file-type-checkboxes from Summary, Authorship and Zoom #2173

Merged
merged 29 commits into from
Apr 23, 2024

Conversation

sopa301
Copy link
Contributor

@sopa301 sopa301 commented Mar 25, 2024

Part of #2001.

Proposed commit message

Extract c-file-type-checkboxes from Summary, Authorship and Zoom

The checkbox collection is duplicated across many components.

Let's abstract them into a separate file.

Other information

Progress:

  • Extract from Summary
  • Extract from Authorship
  • Extract from Zoom

@sopa301 sopa301 requested a review from a team March 26, 2024 14:26
@sopa301 sopa301 marked this pull request as ready for review March 26, 2024 14:26
Copy link
Contributor

@georgetayqy georgetayqy left a comment

Choose a reason for hiding this comment

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

LGTM!

@jonasongg jonasongg requested a review from a team April 15, 2024 04:45
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.

Really great job on this PR @sopa301! LGTM, just a minor nit on the naming/design of the emits of the new component.

frontend/src/components/c-file-type-checkbox.vue Outdated Show resolved Hide resolved
@sopa301 sopa301 requested a review from vvidday April 17, 2024 12:09
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.

Thanks! LGTM

@vvidday vvidday requested a review from a team April 17, 2024 12:12
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! Thanks for the refactor @sopa301! I think we can merge this once the new file is renamed.

frontend/src/components/c-file-type-checkbox.vue Outdated Show resolved Hide resolved
@sopa301 sopa301 changed the title [#2001] Extract c-file-type-checkbox from Summary, Authorship and Zoom [#2001] Extract c-file-type-checkboxes from Summary, Authorship and Zoom Apr 19, 2024
@ckcherry23 ckcherry23 merged commit 693f17d into reposense:master Apr 23, 2024
8 checks passed
Copy link
Contributor

The following links are for previewing this pull request:

Comment on lines +103 to +105
return `${label.fileType}\xA0\xA0`
+ `${label.blankLineCount}\xA0\xA0`
+ `(${label.lineCount - label.blankLineCount})\xA0`;
Copy link
Member

Choose a reason for hiding this comment

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

@sopa301 This must be showing "Line count (Line count - blank line count)", but this is "Blank line count (Line count - blank line count)" instead.

Suggested change
return `${label.fileType}\xA0\xA0`
+ `${label.blankLineCount}\xA0\xA0`
+ `(${label.lineCount - label.blankLineCount})\xA0`;
return `${label.fileType}\xA0\xA0`
+ `${label.lineCount}\xA0\xA0`
+ `(${label.lineCount - label.blankLineCount})\xA0`;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, I'll create a PR to fix it!

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.

4 participants