Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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/dynamic-transitions #1533
feat/dynamic-transitions #1533
Changes from 4 commits
f6852d8
54b4dd8
9976ca8
5638736
1ff7c0f
5444f73
9387fdb
acc9afc
7bfe8ba
34af502
8508590
ea14ccb
ab642e7
6fc34ee
4649b9b
e3bc2cb
e99e73b
1a9fc89
a8cdcb7
45b4d9d
515dc16
21fe122
73fefd2
c55ca8a
8d2f371
88defd7
354cdbc
0e79f78
5a38990
79cdb36
bd21e34
dd009e6
b7524ac
265bfb1
a59bed1
379398f
d17a114
6c6ab7d
e1f03e9
c59e82f
995000f
3e53632
96213ba
105d832
2f831f8
54b595b
dfc6937
f0e09b1
1640d38
82ad5f4
577f634
cce19d1
15f3767
4131ac1
9f43613
56647ce
dfbbfea
aedd888
af56c60
ea968fc
30259cd
25b0f2e
46d46f6
b33b14f
8aaf1ca
2c8b08f
59c2dd8
f2c6cec
a167c32
e4e93d9
6df83bf
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ryceg I like that @Mahmoud-zino has created a global shared type for reuse, but I'v noted we're using
slide
here as the default. Is there a more generic value we can use here? I believe transitions are just functions by default - do we provide a generic function here?This may tie into another issue I'll raise in the PR thread in regards to removing animations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@endigo9740 yeah I looked into it, this will work with most transitions but not
draw
orcrossfade
because they take different parameters. I think the only way to make it really generic is by usingany
...There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can use a union type;
It's not pretty, and it doesn't support custom transitions that include additional params (although you could slap a
| Record<string | any>
on the end to accommodate that) but it does workThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@endigo9740 @ryceg Is using
object
a bad idea? I achieved the same functionality by changing the params to object and it works.and like this, we are not dependent on
slide
anymore.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typescript works by narrowing wider types; the
object
type simply tells Typescript that it's not one of the other primitives. Both are technically "correct", but you're going to be more accurate with a union type (accuracy in the sense of trying to make your types resemble the corresponding data).Try it out- with the union type you'll get intellisense on the object, and it'll narrow down the possible choices as you add props that don't exist on some of the interfaces.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Mahmoud-zino I'm not sure another prop would help since the transition directives are still applied to the elements. We need to alter the value passed to the
transition:in
andtransition:out
, correct?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes and no, we can do something like this repl.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I like it, but there's a lot of boilerplate. We definitely don't want that to live within every component. It would need to be abstracted for reuse by all components.
I'm was thinking something more like how we handle dynamic actions:
https://github.com/skeletonlabs/skeleton/blob/dev/packages/skeleton/src/lib/components/Avatar/Avatar.svelte#L20
If the action is not present, we still need to pass something to
use:
directive, so we pass an empty function:https://github.com/skeletonlabs/skeleton/blob/dev/packages/skeleton/src/lib/components/Avatar/Avatar.svelte#L59
In practice that would look something like this:
I don't like this though, we're then forcing end users to pass a bunch of extra noise.
This makes me wonder if we build in and include our own custom transition called
disabled
:Then allow users to provide this like so:
Then if we can make params optional, it could be:
But this requires importing
disabled
to use it and I'm not sure this guarantees there won't be a performance hit, even if it's minor.Really stumped on this one, it's a tough one to figure out. The limitation here is that Svelte doesn't seem to support dynamic transitions well - so in some ways it's an upstream issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@endigo9740
I like the disabled Idea, but we don't have to return anything from it. Svelte only cares about the function parameters having the correct type.
I don't know how yet but I will try to benchmark both approaches and check the performance.
This might take some time, but I will report back when I have something.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not a huge fan of
disabled
being a function- traditionallydisabled
is a boolean, and there's less friction if we don't require them to import thedisabled
function just to pass it in. We could always add in adisabled
flag in an extended interface very easily, and this would also afford the ability to toggle it.