Use the toggle attribute of the Todo components #5124

lukasmoellerch · 2020-07-10T09:45:49Z

Modifies the "Immutable Data" example on the svelte website and removes the toggle attribute that was declared. Previously the REPL displayed the following warnings:

ImmutableTodo has unused export property 'toggle'. If it is for external reference only, please consider using export const toggle (ImmutableTodo.svelte:8:12)

MutableTodo has unused export property 'toggle'. If it is for external reference only, please consider using export const toggle (MutableTodo.svelte:6:12)

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

It's really useful if your PR relates to an outstanding issue, so please reference it in your PR, or create an explanatory one for discussion. In many cases, features are absent for a reason.
This message body should clearly illustrate what problems it solves. If there are related issues, remember to reference them.
Ideally, include a test that fails without this PR but passes with it. PRs will only be merged once they pass CI. (Remember to npm run lint!)

Tests

Run the tests with npm test or yarn test)

pngwn · 2020-07-29T17:45:45Z

Is there any precedent in the other tutorials? Should we default to events or callback functions? Or maybe it doesn't matter. We could just remove the toggle prop as a fix for this as well.

lukasmoellerch · 2020-07-30T14:55:32Z

@pngwn

Is there any precedent in the other tutorials? Should we default to events or callback functions? Or maybe it doesn't matter. We could just remove the toggle prop as a fix for this as well.

There are several examples where a custom event dispatcher is used, I haven't found any where they are passing a function as a prop directly. I would prefer either passing the function as a prop or using a custom event dispatcher because it provides semantics to the interface of the todo component. But your solution is fine as well.

tanhauhau

i would prefer removing the export let toggle in both MutableTodo and ImmutableTodo

stale · 2021-06-26T19:03:29Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

Use the toggle attribute of the Todo components

393ac4c

mindrones added the site: examples label Jul 10, 2020

Conduitry mentioned this pull request Oct 1, 2020

Fix: unused export property toggle #5478

Closed

4 tasks done

tanhauhau requested changes Nov 18, 2020

View changes

stale bot added the stale-bot label Jun 26, 2021

pngwn added site and removed site: examples labels Jun 26, 2021

stale bot removed the stale-bot label Jun 26, 2021

pngwn added docs and removed site labels Jun 27, 2021

lukasmoellerch added 2 commits Jun 29, 2021

Apply suggestion

2b0692d

Merge branch 'sveltejs:master' into fix-example-immutable-data

1b749fd

sveltejs / svelte

Use the toggle attribute of the Todo components #5124

Use the toggle attribute of the Todo components #5124

lukasmoellerch commented Jul 10, 2020 •

edited

pngwn commented Jul 29, 2020

lukasmoellerch commented Jul 30, 2020

tanhauhau left a comment

stale bot commented Jun 26, 2021

May	JUN	Jul
	30
2020	2021	2022

sveltejs / svelte

Sponsor sveltejs/svelte

Use the toggle attribute of the Todo components #5124

Use the toggle attribute of the Todo components #5124

Conversation

lukasmoellerch commented Jul 10, 2020 • edited

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

Tests

pngwn commented Jul 29, 2020

lukasmoellerch commented Jul 30, 2020

tanhauhau left a comment

stale bot commented Jun 26, 2021

lukasmoellerch commented Jul 10, 2020 •

edited