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

feat: Display directive #14795

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Conversation

adiguba
Copy link
Contributor

@adiguba adiguba commented Dec 21, 2024

WIP : Another possible solution for #9976

This add a new #display directive for DOM elements.
This directive will allow to show/hide an element using the Svelte transition API, without destroying it.

<div transition:slide #display={visible}>
   ...
</div>

=> When visible is "falsy", the element will be hidden using a display: none !important.
Otherwise it will be show either by removing the display style, or setting the value of style:display.

Note : I known that #display={...] is not usual for directive but I think it's more expressive...
In any case we could replace it with something like svelte:display={...}

It's still a work in progress, and need some works that I can do if the syntax of this PR is accepted...

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.
  • If this PR changes code within packages/svelte/src, add a changeset (npx changeset).

Tests and linting

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

Copy link

changeset-bot bot commented Dec 21, 2024

⚠️ No Changeset found

Latest commit: 272f7d4

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@Rich-Harris
Copy link
Member

preview: https://svelte-dev-git-preview-svelte-14795-svelte.vercel.app/

this is an automated message

Copy link
Contributor

Playground

pnpm add https://pkg.pr.new/svelte@14795

@Thiagolino8
Copy link

Thiagolino8 commented Dec 21, 2024

Wouldn't it be possible for this to be a block?

Kinda like a vue v-show directive equivalent?

{#show visible}
  <p transition:slide>hello world</p>
{/show}

@adiguba
Copy link
Contributor Author

adiguba commented Dec 21, 2024

Wouldn't it be possible for this to be a block?

It could only work by setting the style display property, so it need an element.

And yes it's an equivalent to Vue v-show

@kran6a
Copy link

kran6a commented Dec 21, 2024

Wouldn't it be possible for this to be a block?

Kinda like a vue v-show directive equivalent?

{#show visible}
  <p transition:slide>hello world</p>
{/show}

I would prefer a block too, it works basically like an #if and making it a block means we can place multiple HTML elements inside. It also means users don't have to learn new syntax. Components within the block would get passed the corresponding style prop (e.g: style="display:none!important"). If style prop is already present it would upsert the display property accordingly.

<div #display={visible}></div> seems odd and would introduce new syntax (with its corresponding docs).

Also I don't think this feature is important enough to deserve its own syntax as we can already do this:

<div style:display={visible}></div>

Pair that with a getter, a setter and factory pattern to turn it into something like:

<script>
  const handler = visibility_handler("flex", false); //Create an object with a setter and a getter that switches state between "flex" and "none!important" with an initial value of "none!important" as indicated by the second argument
</script>
`<div style:display={handler.visible}></div>`

Which allows you to do things like:

handler.toggle();
handler.visible = false; //assignment is captured via its setter. handler.visible getter now returns "none!important"
handler.visible = true; //assignment is captured via its setter. handler.visible now returns "flex" as it was memoized
handler.visible = "inline" //assignment is captured via its setter. The initial "flex" value is overridden
handler.hide();
handler.show();

@adiguba
Copy link
Contributor Author

adiguba commented Dec 21, 2024

I would prefer a block too, it works basically like an #if and making it a block means we can place multiple HTML elements inside.

But in order to hide the content, we need an element on which to set display:none.
This will cause problems if the block contains text, @html or Components (there no way to pass a style to any component)...

=> How do you hide that :

{#display visible}
   Hello World !
{/display}

Also I don't think this feature is important enough to deserve its own syntax as we can already do this:

<div style:display={visible}></div>

I known that style:display can be used to show/hide an element, but this does not execute the transitions.

Updated example :
https://svelte.dev/playground/hello-world?version=pr-14795#H4sIAAAAAAAACpVU7Y7bIBB8Feqr5ER3OSdVVVU-x1XVP-0zNP2B8dpCwWDBOr2rlXcvGLCT-5BaISswzO4MG9gxkbSDJE--gxCK_FZa1GQFNUeo18ld0nABJsl_jgk-9Y7nAIuHqK99f29OINBhFTXwGs6URJBo0ySFYZr3WB7kAXnXK41kJEbwGsiZNFp1JPVhGWoqDUeuZHpB_qYGm0sH6n0W1kErfXBUAUhO3PBKANmT9wYpwgr1AGu7XWSLAzuKakBUkijJBGfH_bha78sQvH8XJucSVdvabHFjjBtF5uOnbEXNT4QJasw-bTWv0-mUDp0mB7zhjZ-MdhaTnT3kiTG8EoodU7LUIJ9qVEauZcdKZDNYZIvUmPFmyryAl0ZqbnpBn_zqH4RJjFiOvsg-d3Ih-Zq4wScB-X87uAqbbXyRgxB5KpWE9BVLm00MOCSOc0jeNjkjYRSTogcwXJPRB4ekOZnMPniwo7rlMic76AgdUAW4p3XNZZuTz_0j2X3qHyfc_-v2u3c35UVeB4Z4N90gdHYDYcOUGDpprEqj4xeJtJ_EwxLhETdU8NZaYuCqMQs72cl51K2UrkHbaGvRKFtucsMYC4kqyo6ttgWtc3LT7Nx4frQPs2qQsK9sLp5tAM5LkrsneL57o5FcP-TrZvJi742G4h4-c9zl2W_dm3d776FpgOFqtSb7Mp7b5jFIjpZuAH84jRMVgTIlur29I7vtdrv2fA04aEkCQQDVc9Bx7cvrf571maIqxymfaxjl9eXCKrpp7Kk2hv-BnHwMBb0o5XUZf53_AodSc4G5BQAA

@Leonidaz
Copy link

Leonidaz commented Dec 21, 2024

Wouldn't it be possible for this to be a block?

Kinda like a vue v-show directive equivalent?

{#show visible}
 <p transition:slide>hello world</p>
{/show}

Wouldn't it be possible for this to be a block?

It could only work by setting the style display property, so it need an element.

And yes it's an equivalent to Vue v-show

This is really sweet! Hope in some shape or form this can be incorporated.

I also wonder if this could be a block, this way it would be intuitive and basically only way to do it, vs if someone used and if block and the directive.

Components at the top level would NOT be allowed but could be nested inside elements. As far as transitions, elements within components and snippets should work including any nested elements, same as with the {#if} block. But only the top elements will be hidden / shown.

I think it can also play well with the transition |global option with nested blocks.

I think it would make sense to use the {#if} block instead of creating another one as the idea is basically to "tell" the {#if} block to perform a certain action and instead of removing / adding / mounting / unmounting, it should perform a different action.

Please read to the end as this has been more like a discovery journey.
Or just skip to the end. sorry, looks like github doesn't do links inside comments

<script>
  import { fade } from 'svelte/transition';
  
  let signal = $state(true);
</script>

<!-- toggle #if condition -->
<button onclick={() => signal = !signal}>Fade in / out</button>

<!-- short form with functions predefined -->
{#if:transition=[in, out] signal}
   <!-- every top element will run through in and out callbacks, including inside components and snippets -->
  <div transition:fade>fades in and out</div>
{/if}

<!-- shortest form with a function that returns a tuple with for in and out  -->
{#if:transition=display() signal}
   <!-- every top element will run through in and out callbacks, including inside components and snippets -->
  <div transition:fade>fades in and out</div>
{/if}

<!-- long form inlined functions, in out functions run for every top element -->
{#if:transition=[node => node.removeAttr('style'), node => node.style.display = 'none')] signal}
   <!-- every top element will run through in and out callbacks, , including inside components and snippets -->
  <div transition:fade>fades in and out</div>
{/if}

The syntax for {#if could also be more like event handling, if Svelte ends up adding other #if actions, e.g.

{#if:on:transition=[in, out] signal}

Svelte could provide a super basic, like above, display if action with an import svelte/transition/actions but people would be free to create their own if they want more custom logic. One of the actions could be to remove what's inside if, how it currently works. The passed in node to callbackups, could also be a fake / proxy that could limit what could be done.

with svelte provided actions:

<script>
  import { fade } from 'svelte/transition'
  import { display } from 'svelte/transition/actions';
  
  let signal = $state(true);
</script>

{#if:transition=display() signal}
   <!-- every top element will run through in and out callbacks, , including inside components and snippets -->
  <div transition:fade>fades in and out</div>
{/if}

Simplification

But, maybe even simpler, {#if} is already aware of the fact that it needs to wait on transitions. And so, it would make sense just to tell it what action to perform instead of removing all nodes when transitions finish - based on the state of the if condition true or false. The other, arguably, very useful feature that is gained is that even without any transitions, the if-action can be performed on the top elements, e.g. do something else like hide instead of removing from the DOM.

{#if:action=[truthy, falsy]}
{/if}

a more fleshed out example, and actually, it probably just makes sense to use the same syntax as for the transition: directive and provided / custom functions:

<script>
  import { fade } from 'svelte/transition'
  import { display } from 'svelte/transition/actions';
  // display or any custom function that has to return a tuple of functions for truthy and falsy
  // display() => [(node: HTMLElement) => void, (node: HTMLElement) => void]
  
  // example of what `display` could return: [node => node.removeAttr('style'), node => node.style.display = 'none')] 
  
  let signal = $state(true);
</script>

{#if:action:display signal}
  <div transition:fade>fades in and out</div>
{/if}

<!-- or `action:` can be `use:` as it's shorter and already used in a similar fashion -->
{#if:use:display signal}
  <div transition:fade>fades in and out</div>
{/if}

And to reiterate:

  • All top-level elements would be affected by the action just like with the regular {#if} block.
  • Any nested elements at any level will be able to run transitions.
  • The truthy and falsy functions provided by an action will run for every top-level element
  • Components at the top level are NOT allowed but could be nested inside elements at any other level. Top elements within components and snippets are included.
  • Any element that doesn't have a transition: specified WILL be affected by the action at the end / beginning of transitions (hide / show)
  • If there aren't any elements with transition:, the action will simply be performed on the top-level elements immediately.
  • Overall, allowing if blocks to perform actions other than removing elements / unmounting components

NOTE: when a block mounts, the behavior would be the same as for the current block behavior: the transitions don't run and thus the action is not executed. To run transitions on mount, as per current behavior, in onMount or a "mounting" effect the signal can be changed to cause the transitions to run. (Maybe a mount callback can be introduced to actions, so it can return essentially 3 callbacks: mount, signal_true, signal_false).

@adiguba
Copy link
Contributor Author

adiguba commented Dec 22, 2024

A block will have more constraints : no component at top-level, but also no text content, @html or @render.
=> I just want to toggle the visibility of an element, so I think it should be set on the element.

If #display is too exotic, perhaps we could make a special case for style:display with two values (the display default value, and the visibility) :

<div transition:slide style:display={"block", visible}>
   ...
</div>

@Ocean-OS
Copy link
Contributor

If #display is too exotic, perhaps we could make a special case for style:display with two values (the display default value, and the visibility) :

<div transition:slide style:display={"block", visible}>
   ...
</div>

Perhaps something like style:display:block={condition}?
Or, something more Tailwind-esque: style:display-block={condition}

@Leonidaz
Copy link

A block will have more constraints : no component at top-level, but also no text content, @html or @render. => I just want to toggle the visibility of an element, so I think it should be set on the element.

I mean, you can't apply svelte transitions to text node or to @html.

Actually, my bad, {#if} works with components and snippets, so it probably would work the same way but only with the top level elements as far as the resulting set of elements from render and components.

If #display is too exotic, perhaps we could make a special case for style:display with two values (the display default value, and the visibility) :

<div transition:slide style:display={"block", visible}>
   ...
</div>

I thought about it initially but it seemed too confusing with the regular usage.

@adiguba
Copy link
Contributor Author

adiguba commented Dec 23, 2024

I mean, you can't apply svelte transitions to text node or to @html.

We can't hide them either.
Using a block will imply too much restrictions and confusion.

@Leonidaz
Copy link

Leonidaz commented Dec 23, 2024

I mean, you can't apply svelte transitions to text node or to @html.

We can't hide them either. Using a block will imply too much restrictions and confusion.

These are really weird edge cases, and they can be either wrapped in elements , just not placed in the same block or wrapped by another if block. I think @html should work though if it contains html.

Anyway, I’ll be happy with whichever way ends up being supported.

@webJose
Copy link
Contributor

webJose commented Dec 25, 2024

I'm failing to see how this proposal is better than:

<div style:display={visible ? 'block' : 'none'}>

What am I missing?

@paoloricciuti
Copy link
Member

What am I missing?

Probably this part

This directive will allow to show/hide an element using the Svelte transition API, without destroying it.

@adiguba
Copy link
Contributor Author

adiguba commented Dec 25, 2024

What am I missing?

Transitions !
See the updated example

But maybe this #display could be integrated directly into style:display

  • style:display={value} to simply change the property
  • style:display|transition={value} to change the property via transitions, with true/false value be equivalent to null/"none"

@webJose
Copy link
Contributor

webJose commented Dec 25, 2024

What am I missing?

Transitions ! See the updated example

But maybe this #display could be integrated directly into style:display

  • style:display={value} to simply change the property
  • style:display|transition={value} to change the property via transitions, with true/false value be equivalent to null/"none"

Ah! I see. Yes, that part in userland would be a No No. Ok.

Two things:

  • The # prefix is new syntax. Directives in Svelte are <directive>:, right?
  • This would require element wrappers to apply this to components. Any way of avoiding the wrapper?

@adiguba
Copy link
Contributor Author

adiguba commented Dec 25, 2024

The # prefix is new syntax. Directives in Svelte are <directive>:, right?

Yes it's a new syntax, and I'm not sure this is the best choice...
Other possibilities would be to use something like svelte:display or integrate this into style:display via a modifier...

This would require element wrappers to apply this to components. Any way of avoiding the wrapper?

There is no "standard" way to hide a component...

@Leonidaz
Copy link

What am I missing?

Transitions ! See the updated example
But maybe this #display could be integrated directly into style:display

  • style:display={value} to simply change the property
  • style:display|transition={value} to change the property via transitions, with true/false value be equivalent to null/"none"

Ah! I see. Yes, that part in userland would be a No No. Ok.

Two things:

  • The # prefix is new syntax. Directives in Svelte are <directive>:, right?
  • This would require element wrappers to apply this to components. Any way of avoiding the wrapper?

As far as avoiding a wrapper for components, it would be possible with a block directive (again I don't care which way ends up being supported). Any element with a defined transition, at any nested level regardless whether within a component or snippet, will be animated. And this way, elements only define transitions (vs transitions and directives), and as a developer you decide on the behavior at the block level vs at every single element, including 3rd party svelte components.

@adiguba
Copy link
Contributor Author

adiguba commented Dec 25, 2024

As far as avoiding a wrapper for components, it would be possible with a block directive

How did you hide an unknown component, a text node, or any stuff generated by @html/@render ?

It's work with {#if} because it destroy all the block.
But the goal here is only to hide the node...

@Leonidaz
Copy link

As far as avoiding a wrapper for components, it would be possible with a block directive

How did you hide an unknown component, a text node, or any stuff generated by @html/@render ?

Since unmount would not happen, nor elements would be removed from DOM, display:none is set on the top html elements of the block, whichever those happen to be, in the block itself or inside a component, or inside @render or @html. The only exception is text nodes at that top level, but I would consider it an edge case as they would need to be wrapped in an element in order for them to be hidden shown, otherwise they will remain visible.

@Leonidaz
Copy link

As far as avoiding a wrapper for components, it would be possible with a block directive

How did you hide an unknown component, a text node, or any stuff generated by @html/@render ?

Since unmount would not happen, nor elements would be removed from DOM, display:none is set on the top html elements of the block, whichever those happen to be, in the block itself or inside a component, or inside @render or @html. The only exception is text nodes at that top level, but I would consider it an edge case as they would need to be wrapped in an element in order for them to be hidden shown, otherwise they will remain visible.

This ⬆️ (and the comments above) basically only define the behavior / specification.

As far as implementation, I don't know.

I think it's doable to figure this out with DOM. E.g. a comment for start and end of a block can be inserted and then the top children figured out with something like nextElementSibling. (It's even doable to identify text nodes and wrap / unwrap them with an element in order to hide them as svelte keeps a reference to elements for reactive updates, but not sure if this will cause stuff to break).

As far as transitions, I'm assuming they're registered somewhere if the top block waits for them to finish, no matter the nesting level. So, I'm assuming, it can do the same before making the dom elements invisible. And for triggering the in transitions to run, I'm assuming the transitions are still registered and could still be triggered to run.

For SSR, the block will behave the same way as now either render or not render anything depending on the condition.

@adiguba
Copy link
Contributor Author

adiguba commented Dec 25, 2024

Setting display:none to all elements and wrapping text-node may be complex and broke others stuffs.

Ex : how do you handle this ?

{#display visible}
    <Comp />
{/display}

@Ocean-OS
Copy link
Contributor

Setting display:none to all elements and wrapping text-node may be complex and broke others stuffs.

Ex : how do you handle this ?

{#display visible}
    <Comp />
{/display}

Wrapping it in a <span>/<div> would probably be the best solution.

@webJose
Copy link
Contributor

webJose commented Dec 25, 2024

{#display visible}
    <Comp />
{/display}

How about having Svelte enumerate all top-level elements at the block's position and adding setting the display on each of them?

@Leonidaz
Copy link

Setting display:none to all elements and wrapping text-node may be complex and broke others stuffs.

Ex : how do you handle this ?

{#display visible}
    <Comp />
{/display}

so, as I mentioned, one way is to find the elements inside the components and set the ones at the top as display:none - I don't think this should break anything. Wrapping text nodes not 100% but svelte operates with explicit references to the elements for reactivity and that wouldn't change.

Otherwise, whatever is inside the block could be wrapped in one div with display:none when in the state of hidden, i.e. condition is falsy, and the wrapping div is removed when the condition changes to truthy.

@Ocean-OS
Copy link
Contributor

so, as I mentioned, one way is to find the elements inside the components and set the ones at the top as display:none - I don't think this should break anything. Wrapping text nodes not 100% but svelte operates with explicit references to the elements for reactivity and that wouldn't change.

Otherwise, whatever is inside the block could be wrapped in one div with display:none when in the state of hidden, i.e. condition is falsy, and the wrapping div is removed when the condition changes to truthy.

Wouldn't it be more performant to just change the div's style instead of removing it?

@Leonidaz
Copy link

so, as I mentioned, one way is to find the elements inside the components and set the ones at the top as display:none - I don't think this should break anything. Wrapping text nodes not 100% but svelte operates with explicit references to the elements for reactivity and that wouldn't change.
Otherwise, whatever is inside the block could be wrapped in one div with display:none when in the state of hidden, i.e. condition is falsy, and the wrapping div is removed when the condition changes to truthy.

Wouldn't it be more performant to just change the div's style instead of removing it?

if we're talking about a wrapper div, it's really fast to just move the elements after the last sibling just above the block. I'm not sure performance is a concern here with animations and all, especially compared to the usual destruction and recreation of nodes / components. Otherwise, we introduce another html element that could disrupt the css while the elements are visible (while they're not invisible, it should be fine with reactivity and css doesn't matter). And I think in this case we might as well have devs create their own wrapper.

@Leonidaz
Copy link

Also, if the actions are customizable as I suggested, if a default behavior is not satisfactory, a custom one can provided. As long as the in and out action callback functions get a reference to the previous sibling node and the sibling node after the block (these can be just 2 comments before and after the block) and a function that triggers the transitions to run.

@xeho91
Copy link
Contributor

xeho91 commented Dec 26, 2024

If #display is too exotic, perhaps we could make a special case for style:display with two values (the display default value, and the visibility) :

<div transition:slide style:display={"block", visible}>
   ...
</div>

This one is the most promising one, given the current state of AST nodes in Svelte. I've observed that Svelte core maintainers try to use ESTree specification (for JavaScript syntax) as much as possible, so the newcomers don't need to be confused by new things that doesn't exist neither in HTML or JavaScript.
So, I would change it slightly to look like is an expression tag, with a tuple of two items.

<div transition:slide style:display={["block", visible]}>
    <!-- ... -->
</div>

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.

9 participants