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: improve component binding examples #8931

Conversation

Pacheco95
Copy link

Before submitting the PR, please make sure you do the following

  • It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
  • Prefix your PR title with feat:, fix:, chore:, or docs:.
  • This message body should clearly illustrate what problems it solves.
  • Ideally, include a test that fails without this PR but passes with it.

Tests and linting

  • Run the tests with pnpm test and lint the project with pnpm lint

@benmccann benmccann changed the title chore: Improve component binding examples docs: improve component binding examples Jul 7, 2023
@PuruVJ
Copy link
Collaborator

PuruVJ commented Jul 17, 2023

Thanks a lot for this PR! Here are my thoughts on this 👇

As an "example" point of view, I find duplication much easier to parse, whereas the loop being more concise, would require thinking a bit more, which is not the point of the example. And these examples are primarily for the beginners, and we'd be making parsing harder for them.

If this was an example demonstrating the each block, it'd be fine. Overall, I'd say this change isn't an improvement, necessarily.

@benmccann
Copy link
Member

I could go either way on this. The current version was probably written the way it was for a reason, which I'm assuming is the one @PuruVJ put forth, so I'll lean towards keeping it as is even if it's a bit verbose. Thanks!

@benmccann benmccann closed this Sep 15, 2023
@PuruVJ
Copy link
Collaborator

PuruVJ commented Sep 18, 2023

> I could go either way on this. The current version was probably written the way it was for a reason, which I'm assuming is the one @PuruVJ put forth, so I'll lean towards keeping it as is even if it's a bit verbose. Thanks!

As far as I remember, it was introduced by a first time contributor, and merged by dummdidum. I was opposed to the change, but didn't get to voice it before it was merged.

I prefer the verbose version much more, for something like an example. I'm gonna reopen it, and if you have no objection, merge it 😊

Edit: Uff I got confused with the code, yes, the verbose version is better. Closing the PR again 😅

@PuruVJ PuruVJ reopened this Sep 18, 2023
@PuruVJ PuruVJ closed this Sep 18, 2023
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.

None yet

3 participants