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

Move computed+watchers and class+style #31

Merged
merged 4 commits into from Jan 22, 2020
Merged

Conversation

NataliaTepluhina
Copy link
Member

Warning: class and style needs a check after https://github.com/vuejs/rfcs/blob/attr-fallthrough/active-rfcs/0000-attr-fallthrough.md is merged as currently classes and styles are NOT merged to the single root node attributes

<!-- is able to remain small by not reinventing them. This also -->
<!-- gives you the freedom to use what you're familiar with. -->
<script src="https://cdn.jsdelivr.net/npm/axios@0.12.0/dist/axios.min.js"></script>
<script src="https://cdn.jsdelivr.net/npm/lodash@4.13.1/lodash.min.js"></script>
Copy link
Member

Choose a reason for hiding this comment

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

Does it complicate things to introduce a third party library like lodash in the example? I've worked on a number of projects where they avoided lodash and it could be seen as another thing that people have to learn to understand the example. But maybe not? Curious what your thought is on it.

Copy link
Member Author

Choose a reason for hiding this comment

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

So far I am not changing examples from Vue 2 docs (except moving them to Vue 3 syntax). But I think you're right and I will try to change this one

Copy link
Member Author

Choose a reason for hiding this comment

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

@bencodezen I fixed an example, could you please look at it one more time?

@bencodezen bencodezen added content Issues / PRs related to docs content enhancement New feature or request labels Jan 10, 2020
@bencodezen bencodezen added this to In progress in docs-next-alpha via automation Jan 10, 2020
@bencodezen bencodezen added this to the alpha milestone Jan 10, 2020
@NataliaTepluhina NataliaTepluhina moved this from In progress to Review in progress in docs-next-alpha Jan 14, 2020
Copy link
Member

@znck znck left a comment

Choose a reason for hiding this comment

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

Is the purpose of this PR is only migration or we are considering possible rewrites too?

};
},
computed: {
reversedMessage: function() {
Copy link
Member

Choose a reason for hiding this comment

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

I feel this example is introducing to complex handling of strings which is out of the scope here. We can add a simpler example something like sum of two numbers as computed property.


[Learn how computed properties work with a free lesson on Vue School](https://vueschool.io/lessons/vuejs-computed-properties?friend=vuejs)

In-template expressions are very convenient, but they are meant for simple operations. Putting too much logic in your templates can make them bloated and hard to maintain. For example:
Copy link
Member

Choose a reason for hiding this comment

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

The above example makes sense, but in my opinion, we introduce computed properties as a mechanism to get derived values. The introduction as a solution to prevent complex expressions in templates does not sound that impressive.

@phanan
Copy link
Member

phanan commented Jan 19, 2020 via email

@NataliaTepluhina
Copy link
Member Author

I agree with @phanan, let's follow the idea of small iterations. However, I agree with @znck too on simplifying the example. I created a follow-up issue to prevent losing Rahul's comments while PR will get merged: #38

docs-next-alpha automation moved this from Review in progress to Reviewer approved Jan 22, 2020
@bencodezen bencodezen merged commit 3712e66 into master Jan 22, 2020
docs-next-alpha automation moved this from Reviewer approved to Done Jan 22, 2020
@bencodezen
Copy link
Member

Sorry for the delay @NataliaTepluhina. Thanks for your work on this!

@NataliaTepluhina NataliaTepluhina deleted the move-computed-watchers branch May 4, 2020 04:17
@bencodezen bencodezen mentioned this pull request Jul 6, 2020
25 tasks
@Xenonym Xenonym mentioned this pull request Jun 22, 2021
skirtles-code pushed a commit that referenced this pull request Jun 22, 2021
axios was added in #31, but is now unused as code examples were moved
to CodePen in #57.

Let's remove axios.
TalexDreamSoul pushed a commit to Talexs/docs that referenced this pull request Apr 17, 2022
moHaHa pushed a commit to moHaHa/vuejs-docs that referenced this pull request May 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
content Issues / PRs related to docs content enhancement New feature or request
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants