Skip to content

Added slot to todo list component #763

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

Merged
merged 2 commits into from
Jan 10, 2021

Conversation

hitautodestruct
Copy link
Contributor

Added slot to todo list component to emphasize the fact that you can replace the content with custom content.
As the sentence states, this won't work without scoped slots.

Description of Problem

The code does not make sense for the sentence that follows it:

We might want to replace the slot to customize it on parent component

Proposed Solution

Added <slot> wrapper to emphasize that there is a slot to customize

Additional Information

I had to read this example a few times to understand which slot is being customized in order to understand.

Added slot to todo list component to emphasize the fact that you can replace the content with custom content.
As the sentence states, this won't work without scoped slots.
Copy link
Contributor

@skirtles-code skirtles-code left a comment

Choose a reason for hiding this comment

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

Sorry for the delay in getting back to you.

I agree that the current wording is confusing. I think adding a <slot> does fix one source of confusion but I fear it may introduce a new one. In general, I think we should try to avoid having examples of code that doesn't work, as I think readers can easily get lost that way. Sometimes it's necessarily to illustrate a point but I think we may be able to find an alternative here.

Perhaps the sentence that follows this example could be reworded to something like this:

We might want to replace the `{{ item }}` with a `<slot>` to customize it on parent component:

What do you think? Would that have made it clearer for you?

@NataliaTepluhina
Copy link
Member

I've made a change recommended with @skirtles-code 👍🏻

@NataliaTepluhina NataliaTepluhina merged commit 705fea0 into vuejs:master Jan 10, 2021
@hitautodestruct
Copy link
Contributor Author

Hey

Yes, I think that will do it.
I know I had a hard time with slots in the beginning and this stage always confused me, especially with the example code.

Why would <slot>{{ item }}</slot>, not work?
That would just give me the option to overwrite the {{ item }} with a custom slot, wouldn't it?

@Alex-Sokolov
Copy link
Contributor

@NataliaTepluhina @hitautodestruct there is a problem with this change — {{ item }} not showing
Снимок экрана 2021-01-10 в 14 00 49

@hitautodestruct
Copy link
Contributor Author

hitautodestruct commented Jan 10, 2021

@Alex-Sokolov What if we remove the spaces around item making it {{item}}?

@skirtles-code
Copy link
Contributor

@Alex-Sokolov I'll put in a PR shortly to fix that. VuePress interprets template syntax in inline code so it needs escaping. Something similar was required to document the delimiters option.

@skirtles-code
Copy link
Contributor

Why would <slot>{{ item }}</slot>, not work?
That would just give me the option to overwrite the {{ item }} with a custom slot, wouldn't it?

@hitautodestruct Yes, you're correct it would. Describing it as 'code that doesn't work' wasn't ideal wording on my part. But you'd end up with the exact same content for each of the items, so in practice it wouldn't be a useful todo-list. That makes it difficult for readers to use it as a reference point because it isn't doing something relatable.

I think the intention of that section is to start with an example that doesn't use slots at all and introduce a working scoped slot. The sentence you highlighted originally was confusing but that's now changed. Had we introduced a <slot> into the initial example then I think we'd also have needed to change various other parts of the text to match.

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

Successfully merging this pull request may close these issues.

4 participants