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

[#1955] Usage of directive shorthand syntax in .vue files #2290

Merged
merged 20 commits into from
Feb 15, 2025

Conversation

JoanneHing
Copy link
Contributor

Fixes #1955

Proposed commit message

Use directive shorthands in .vue files

This refactor removes explicit Vue directives and replaces them with
shorthand syntax, following Vue's Style Guide practices to improve
readability and reduce verbosity.

Changes include:
- Using `:` instead of `v-bind:`
- Using `@` instead of `v-on:`
- Using `#` instead of `v-slot`

@JoanneHing JoanneHing self-assigned this Feb 10, 2025
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.

Good work! Could we also change line 371 in learningBasics.md to reflect this change?

I personally think this change will not add too much of a learning curve to new developers, but it'd be good to see what you guys think @reposense/active-reviewers-1

@JoanneHing
Copy link
Contributor Author

Good work! Could we also change line 371 in learningBasics.md to reflect this change?

I personally think this change will not add too much of a learning curve to new developers, but it'd be good to see what you guys think @reposense/active-reviewers-1

Sure I can update the documentation! About the learning curve I saw relevant discussion on the issue that I linked too. I agree with Marcus opinion that it might be a bit ambiguous for developer that are unfamiliar to Vue, but the learning curve is not steep. Updating this also align with the Vue guide to use shorthand syntax for better code quality, and I agree that is a good practice for new developer like me myself to adapt to it early.

@sopa301 sopa301 requested a review from a team February 10, 2025 15:56
Copy link
Contributor

@CYX22222003 CYX22222003 left a comment

Choose a reason for hiding this comment

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

LGTM!


For more detailed information, please refer to the [Vue Guide](https://vuejs.org/api/built-in-directives.html#v-bind).
</box>

Copy link
Contributor

Choose a reason for hiding this comment

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

The document is correctly updated.

template(v-for="slice in day.commitResults", :key="slice.hash")
c-zoom-commit-message(:slice="slice",
:selected-file-types="selectedFileTypes", :file-type-colors="fileTypeColors",
:info="info", :show-diffstat="showDiffstat")
Copy link
Contributor

Choose a reason for hiding this comment

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

Tested on local machine, changes does not break the current frontend page.

@CYX22222003 CYX22222003 requested review from a team and jonasongg February 11, 2025 00:00
Copy link
Contributor

@joeng03 joeng03 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
Copy link
Contributor

@JoanneHing Thanks for the documentation change! I was actually referring to changing v-bind:class to :class in this line 371:

add the following to its v-bind:class attribute map

But I think what you wrote is also a great addition!

@JoanneHing
Copy link
Contributor Author

@JoanneHing Thanks for the documentation change! I was actually referring to changing v-bind:class to :class in this line 371:

add the following to its v-bind:class attribute map

But I think what you wrote is also a great addition!

Oh! I see your point! Let me update on this!

@jonasongg
Copy link
Contributor

LGTM! thank you :)

@sopa301 sopa301 merged commit 1c4706a into reposense:master Feb 15, 2025
11 checks passed
Copy link
Contributor

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.

Usage of directive shorthand syntax
5 participants