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

[v3] Fix issue where class directives wouldn't work with spread props and class prop #3242

Merged
merged 4 commits into from
Jul 23, 2019

Conversation

umanghome
Copy link
Contributor

@umanghome umanghome commented Jul 14, 2019

Currently, if there is any spread prop along with a dynamic class prop on the element, the class: directives stop working once the value of the variable that class was dependent on changes. This is because it sets the class prop of the DOM element directly, removed the class attached by the class: directive. And since the variable affecting the directive hasn't changed, a class for it is not applied again.

This should not happen, and class: directive should work just fine with spread props.

I have added tests in this PR as well, which were failing with v3.6.7, but are fixed with the succeeding commits.

REPL

The generated update functions, with version 3.6.7:

WithoutSpread.svelte

p(changed, ctx) {
	if (default_slot && default_slot.p && changed.$$scope) {
		default_slot.p(get_slot_changes(default_slot_1, ctx, changed, null), get_slot_context(default_slot_1, ctx, null));
	}

	if (!current || changed.allClasses) {
		attr(div, "class", ctx.allClasses);
	}

	if ((changed.allClasses || changed.reverse)) {
		toggle_class(div, "reverse", ctx.reverse);
	}
}

WithSpread.svelte

p(changed, ctx) {
	if (default_slot && default_slot.p && changed.$$scope) {
		default_slot.p(get_slot_changes(default_slot_1, ctx, changed, null), get_slot_context(default_slot_1, ctx, null));
	}

	set_attributes(div, get_spread_update(div_levels, [
		(changed.allClasses) && { class: ctx.allClasses },
		(changed.attributes) && ctx.attributes
	]));

	if (changed.reverse) {
		toggle_class(div, "reverse", ctx.reverse);
	}
}

The generated update functions, with changes in this PR:

WithoutSpread.svelte

p(changed, ctx) {
	if (default_slot && default_slot.p && changed.$$scope) {
		default_slot.p(get_slot_changes(default_slot_1, ctx, changed, null), get_slot_context(default_slot_1, ctx, null));
	}

	if (!current || changed.allClasses) {
		attr(div, "class", ctx.allClasses);
	}

	if ((changed.allClasses || changed.reverse)) {
		toggle_class(div, "reverse", ctx.reverse);
	}
}

WithSpread.svelte

p(changed, ctx) {
	if (default_slot && default_slot.p && changed.$$scope) {
		default_slot.p(get_slot_changes(default_slot_1, ctx, changed, null), get_slot_context(default_slot_1, ctx, null));
	}

	set_attributes(div, get_spread_update(div_levels, [
		(changed.allClasses) && { class: ctx.allClasses },
		(changed.attributes) && ctx.attributes
	]));

	if ((changed.allClasses || changed.reverse)) {
		toggle_class(div, "reverse", ctx.reverse);
	}
}

@umanghome
Copy link
Contributor Author

v3 version of #3236. Solves #2707.

@umanghome umanghome changed the title [v3] Fix issue where class directives wouldn't work with spread attributes and class attribute [v3] Fix issue where class directives wouldn't work with spread prop and class prop Jul 14, 2019
@umanghome umanghome changed the title [v3] Fix issue where class directives wouldn't work with spread prop and class prop [v3] Fix issue where class directives wouldn't work with spread props and class prop Jul 14, 2019
@Rich-Harris Rich-Harris merged commit 15c57e1 into sveltejs:master Jul 23, 2019
@Rich-Harris
Copy link
Member

Brilliant fix, thank you. Will come back to the v2 one soon, it'll take some git-fu

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.

2 participants