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

[docs] clarified definition of "local transition" in tutorial #6895

Merged
merged 4 commits into from
Nov 26, 2021

Conversation

mjvandermeulen
Copy link
Contributor

I was confused while reading the tutorial about local transitions. The tutorial talks about removing items and words the transition in those terms, which makes the transition block a parent. BUT... then it talks about the local directive, which belongs to the block with the transition itself.

To avoid confusion I have pretty much copied the wording from the docs verbatim (https://svelte.dev/docs, search for "Local transitions only play"), since I find the wording in the docs very clear.

I have not created an issue. Please let me know if this would still be helpful or is still needed.

I was confused while reading the tutorial about local transitions. The tutorial talks about removing items and words the transition in those terms, which makes the transition block a parent. BUT... then it talks about the local directive, which belongs to the block with the transition itself.

To avoid confusion I have pretty much copied the wording from the docs verbatim (https://svelte.dev/docs, search for "Local transitions only play"), since I find the wording in the docs very clear.
I saw too late that I had added extra whitespace. This commit needs to be squashed with previous commit, before merge.
@benmccann benmccann changed the title [fix] Corrected confusing definition of the "local transition" in the tutorial. [docs] clarified definition of "local transition" in tutorial Nov 4, 2021
Copy link
Member

@bluwy bluwy left a comment

Choose a reason for hiding this comment

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

I actually find the wording in the tutorial more concise than the docs. What if we word it slightly differently, like:

We can achieve this with a *local* transition, which only plays when the immediate parent block of the transition is added or removed:

(added "of the transition")

@mjvandermeulen
Copy link
Contributor Author

@bluwy. Your definition is not correct: the Doc states correctly that local transitions only play when the block they belong to is added or destroyed. The definition given in the tutorial is only correct in the context of the tutorial.

For clarification: The line I removed in the PR could have read (notice emphasis)
- We can achieve this with a *local* transition, which only plays when the immediate parent block **of the item we have been focussing on** is added or removed:

To make the definition more generalized I have changed it to the wording in the PR:
Local transitions only play when the block they belong to is created or removed, not when parent blocks are created or removed.

@bluwy
Copy link
Member

bluwy commented Nov 8, 2021

Ah alright. I think I see your point now. I still think the definition in the tutorial should be curated for that context specifically, that's probably the point of a tutorial. What if we word it something like:

- We can achieve this with a *local* transition, which only plays when the item itself is added or removed:

@mjvandermeulen
Copy link
Contributor Author

@bluwy I would be happy to see your last wording implemented. I have one hesitation though, because of the ambiguous meaning of 'item' in this context.

To clarify, the HTML we are talking about here is:

<div transition:slide|local>
  {item} e.g.: "one"
</div>

In the reader's mind removing the 'item' could mean removing the div's innerHTML, or the whole div.

I agree with @bluwy that the wording should be curated for this context. My new suggestion is the following:

We can achieve this with a *local* transition, which only plays when the block with the transition itself is added or removed.

But, again, I'm happy with implementing @bluwy last suggestion.

@bluwy
Copy link
Member

bluwy commented Nov 10, 2021

I like your new suggestion, and I agree that item could be ambiguous too. Feel free to make your change!

The improved definition applied here is the result of the discussion started by this PR.
The original colon from the tutorial got lost in all my edits, but now is found and happily returned.
@mjvandermeulen
Copy link
Contributor Author

@bluwy It's been 15 days since you approved my last changes. I'm not sure if the ball is in my court, or if we just have to be patiently waiting for someone with write access to merge my pull request. Please inform.

@bluwy
Copy link
Member

bluwy commented Nov 26, 2021

I wanted to have another maintainer's opinion on this, but since it's a small change which I think makes sense, I'll go ahead and merge this. Thanks for the PR!

@bluwy bluwy merged commit 81fc3f8 into sveltejs:master Nov 26, 2021
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.

3 participants