-
Notifications
You must be signed in to change notification settings - Fork 164
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
Conversation
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.
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. |
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!
|
||
For more detailed information, please refer to the [Vue Guide](https://vuejs.org/api/built-in-directives.html#v-bind). | ||
</box> | ||
|
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.
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") |
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.
Tested on local machine, changes does not break the current frontend page.
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
@JoanneHing Thanks for the documentation change! I was actually referring to changing
But I think what you wrote is also a great addition! |
Oh! I see your point! Let me update on this! |
LGTM! thank you :) |
The following links are for previewing this pull request:
|
Fixes #1955
Proposed commit message