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

[3.x] Use slot scopes without destructuring #601

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Jade-GG
Copy link
Collaborator

@Jade-GG Jade-GG commented Oct 21, 2024

The destructuring made it harder to add functionality to vue components, as it meant you also had to overwrite the view containing the slot-scope just because we defined every individual thing unnecessarily.

This is of course an extremely breaking change, and should also be done in other packages. Just making sure we actually want this before I do that though 😉

indykoning
indykoning previously approved these changes Oct 22, 2024
@royduin
Copy link
Member

royduin commented Oct 23, 2024

Do we really want this everywhere? I'm not sure if this is better. Thoughts @indykoning and @BobWez98?

@Jade-GG
Copy link
Collaborator Author

Jade-GG commented Oct 23, 2024

Do we really want this everywhere? I'm not sure if this is better. Thoughts @indykoning and @BobWez98?

What seems nice to me here is that it's much easier to keep variable scopes clear. When you see just a variable named data in a template you might not always know what it's from. Plus, you don't have to worry about variable name clashing in this type of setup :)

There's definitely some places where it's overkill though, I've done them all in this PR for consistency. If we want to break from the consistency to keep some templates cleaner, then I think we should probably define some sort of rule when to use destructuring and when not to use it.

@BobWez98
Copy link
Collaborator

I think this is nice whenever there's a lot of variables. Like here; https://github.com/rapidez/core/pull/601/files#diff-761aeeeb279b8890378bf9eb7f9956bba2d671c1e71e2175de4b768e0ca28b7aL9 .
Whenever there's only one variable used i don't see why we should do this.

@royduin
Copy link
Member

royduin commented Oct 23, 2024

With V3 (just merged in master) there are some changes (that's why there are so many conflicts no) and this is used on some places. It's also possible to rename data to something else like here: https://github.com/rapidez/checkout-theme/pull/118/files#diff-e51d8c1159c452b5ecbf3b7be1374b69bf4944493c994e48db5270f11e6b2dd6R25

Where do we like it and where don't we like it?

@royduin royduin requested a review from indykoning October 23, 2024 15:00
@royduin
Copy link
Member

royduin commented Dec 5, 2024

Looking forward to a follow-up on this

@royduin royduin marked this pull request as draft December 5, 2024 09:41
Co-authored-by: Roene Verbeek <85165259+Roene-JustBetter@users.noreply.github.com>
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.

5 participants