feat/dynamic-transitions

[Mahmoud-zino]

Linked Issue

Closes #1322
Closes #1694

Description

1. Added dynamic Transition types.
2. replaced accordion's transitions with dynamic transitions.

Changsets

feat: Implemented dynamic transitions to the Accordion component.

Checklist

Please read and apply all contribution requirements.

• This PR targets the dev branch (NEVER master)
• Documentation reflects all relevant changes
• Branch is prefixed with: docs/, feat/, chore/, bugfix/
• Ensure code linting is current - run pnpm format
• All test cases are passing - run pnpm test
• Includes a changeset (if relevant; see above)

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
skeleton-docs	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Jul 14, 2023 5:05pm

[Mahmoud-zino]

@endigo9740
As we talked about before, I've applied the changes to the accordion to check with you if the changes are ok.

Some Notes and clarifications about the changes

1. some of the implementation details are not exactly as we discussed because I tried them and found the current implementation somewhat better. (please let me know if I have to change anything)
2. I used any[] for the transition function parameters. is it ok or is there a better solution maybe?
3. I feel like the transition changes need more clarification in the docs, but I don't know if it is a good idea to include a transition section in every component we change or just include it somewhere else.
4. I had to change the AccordionItem.test to include the transition parameter because it can't get it from the parent context. Is it correct or should I change it and how?

Thank you for taking the time to check this.

[endigo9740]

Looking good so far, to respond to your questions:

1. Per the way you implemented this - I like it overall, but the reactive properties require a lot more boilerplate. Just because we can make these reactive, I'm not sure we should. Typically animations are going be hard set upfront. The inherited approach I suggested originally would handle this use case with seemingly a lot less code. Were there any specific concerns with this approach? Keep in mind the maintainability for this approach across a slew of components.
2. We definitely don't want to use any - I'll bring in @ryceg to provide feedback for for typing though. But if we need a custom type defined we can do so in the same place as CssClasses (root of the package /lib). Rhys let us know your thoughts here.
3. I like the idea of indicating per component which includes dynamic transition support, but then linking to a centralized doc page since the process should be uniform. Let me think about how we structure that. I'm happy to handle that portion when we get there.
4. Don't stress too much about the test cases, ours are in dire need of updates. So just do what you need to do to get it working for now.

Also thanks for follow our pattern of implementing a deprecated array. However, go ahead and move the depreciated duration prop itself to the end of the list of props within the component. This will help get it out of the way. Then we'll aim to purge it in the next major release when breaking changes are allowed.

[Mahmoud-zino]

Were there any specific concerns with this approach?

no not really, I just thought we wanted them to still be reactive.

I will go ahead and follow your tips for now and commit first when I get more info about the types. after that should I wait for another review or just implement the rest of the components?

[endigo9740]

@ryceg is based near Australia so he'll be on in a few hours - let's wait for his feedback and then I'd like to do one more full audit with all changes in place. Once that's settled we can move forward with other components.

We won't have this in time for tomorrow's release so take your time, no rush. We'll have a couple weeks to get this ready.

[ryceg]

Awesome stuff! Just a couple typing fixes, plus the @deprecated syntax

EDIT: Oh, plus you might need to import slide

[endigo9740]

Thanks @ryceg!

[Mahmoud-zino]

@ryceg Thank you very much, this looks awesome!

[Mahmoud-zino]

@endigo9740 I have implemented the fixes mentioned by @ryceg .
please let me know if I missed something 👍.

[endigo9740]

@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.

[Mahmoud-zino]

@endigo9740 yeah I looked into it, this will work with most transitions but not draw or crossfade because they take different parameters. I think the only way to make it really generic is by using any...

[ryceg]

We can use a union type;

transition: (...args: Parameters<typeof blur | fade | fly | slide | scale>) => TransitionConfig

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 work

[Mahmoud-zino]

@endigo9740 @ryceg Is using object a bad idea? I achieved the same functionality by changing the params to object and it works.

export interface TransitionSettings {
	transition: (node: HTMLElement, params: object) => TransitionConfig;
	params: object;
}

and like this, we are not dependent on slide anymore.

[ryceg]

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.

[endigo9740]

@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 and transition:out, correct?

[Mahmoud-zino]

yes and no, we can do something like this repl.

[endigo9740]

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:

<SomeComponent
    transitionIn={{transition: () => {}, params: {}}}
    transitionOut={{transition: () => {}, params: {}}}
/>

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:

function disabled(node, { duration }) {
    return { duration: 0, css: t => `` };
}

Then allow users to provide this like so:

<SomeComponent
    transitionIn={{transition: disabled, params: {}}}
    transitionOut={{transition: disabled, params: {}}}
/>

Then if we can make params optional, it could be:

<SomeComponent
    transitionIn={{transition: disabled}}
    transitionOut={{transition: disabled}}
/>

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.

[Mahmoud-zino]

@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.

[ryceg]

I'm not a huge fan of disabled being a function- traditionally disabled is a boolean, and there's less friction if we don't require them to import the disabled function just to pass it in. We could always add in a disabled flag in an extended interface very easily, and this would also afford the ability to toggle it.

[endigo9740]

Additionally @Mahmoud-zino @ryceg we should make considerations for accessibility here - which includes the ability for users to reduce or disable animations. Should we allow a a null or undefined value to be passed in favor of the object? I'm not sure which the transition directives can support off hand.

I faintly recall that either the transition directive -or- the transitions themselves are built to support prefers-reduced-motion. Perhaps this is already handled for us as long as users use the canned transitions. But what if they don't - what if a custom transition is used that isn't built to support this?

Reference:
https://geoffrich.net/posts/accessible-svelte-transitions/

I'll need to do a bit more reading on this subject, but wanted to encourage others to join in.

---

EDIT

From the post above

Since Svelte's built-in transitions are applied in CSS, we can disable them in CSS. The prefers-reduced-motion media query will detect if the user has requested reduced motion in their device settings. You can add the following to your global styles to disable all CSS animation.

This means we can probably rely on Svelte's canned transitions to adhere to prefers-reduced-motion, but I'm lacking knowledge on what options there are for transitions - if JS animation is possible, then this may not be enough.

[ryceg]

So under the hood, the transitions apply CSS as t moves from 0 to 1. If we implemented a $prefersReducedMotion store as Geoff Rich suggests, we can simply use a ternary to apply the correct duration for the fallback property. i.e.

export let transitionIn: TransitionSettings = { transition: slide, params: { duration: $prefersReducedMotion ? 0 : 200 } };

Thus, no tweening occurs between 0 and 1, so the duration would be instant- no motion sickness.

This allows people to still customize the transition behaviour to account for the media query manually, but protects end-users if they just go with the default.

[endigo9740]

@Mahmoud-zino @ryceg I commented on the type thread above.

I do like the suggestion of the $prefersReducedMotion store. I do think we should go ahead and implement that. We don't currently have a location for a global store yet, so I'm leaning towards /src/lib/stores.ts.

import { writable, type Writable } from 'svelte/store';

export const prefersReducedMotion: Writable<boolean> = writable(false);

As discussed previously, we'll need a new "Animations" or "Transitions" page in Docs > Essentials > Transitions (or Animations) that covers the process of modifying transitions. This store can be covered there.

I'm happy to introduce this once we've finalized on the type issue above. So type, doc, and then I think we're good to move forward with other components. Given how quickly things are changing lately I might suggest further component updates come as their own dedicated PRs

[ryceg]

export type DefaultTransitionParams = Parameters<typeof blur | typeof fade | typeof fly | typeof slide | typeof scale>[1] &
	AdditionalTransitionProps;
export type TransitionParams = (DefaultTransitionParams | Record<string, any>) & AdditionalTransitionProps;

interface AdditionalTransitionProps {
	/** Disable the transition completely. */
	disabled?: boolean;
	/**
	 * By default, animations will be disabled if the media query `prefers-reduced-motion` is present.
	 * There are many reasons why you might want to override this behavior, such as for minor animations.
	 */
	ignoreReducedMotion?: boolean;
}

export interface TransitionSettings {
	transition: (node: Element, params?: TransitionParams) => TransitionConfig;
	params?: TransitionParams;
}

This is an opinionated way of doing it, opting for people having to explicitly set to ignore reducedMotion- this would be my preference, as accessibility should be the default, and people should opt in to patterns that may reduce accessibility. The disabled prop also allows for it to be toggled on an off, an escape hatch.

[Mahmoud-zino]

@ryceg

export type DefaultTransitionParams = Parameters<typeof blur | typeof fade | typeof fly | typeof slide | typeof scale>[1] & AdditionalTransitionProps;

Svelte has BlurParams, FadeParams .... etc, which we can use instead of taking the parameters from the function and then extracting the type of the second parameter.

I like the idea of extending the interface to take disabled and ignoreReducedMotion. I will implement these changes and wait for you to take a look at them in the code. Thanks

[changeset-bot]

🦋 Changeset detected

Latest commit: 6df83bf

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@skeletonlabs/skeleton	Minor

Not sure what this means? Click here to learn what changesets are.

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

[endigo9740]

@Mahmoud-zino I've done a follow up per the items mentioned in my prior review:

(InputChips) use these durations are required for animations like flip...

Gotcha, we can leave that for now then.

(per Modal's use of duration) no idea why I missed that. I deleted them, thank you for the reminder.

Thanks for adjusting this - looks good.

(per the contributor docs) the draft is done. 👍

Looks great, I just committed a few tweaks and adjustments to the text and provided a more detailed example snippet. But overall it does what it needs to do. Great job.

Additional Items

1. I reviewed your solution for the Drawer opacityTransition. I think this will suffice for now. It's currently the exception to the rule of how transitions are handled, but it's a unique use case.
2. I noted that Toasts still implement duration due the fact it implements the custom crossfade animation by (Cory) @saturnonearth. Unfortunately this is an exception to our new standard, so I'm leaning towards us reverting back to a standard animation in the meantime (there is a canned crossfade transition available). We shouldn't delete the custom transition, but it should be abstracted and removed from the component and moved to it's own standalone file in /src/lib/transitions/crossfade.ts for now.

If you want to take a stab at converting this the custom Crossfade transition to a proper reusable JS transition per the Svelte Docs you can @Mahmoud-zino, but otherwise I think we set it aside and revisit custom transitions in a dedicated pass in the future. Here's the docs if you want to reference how that works:
https://svelte.dev/tutorial/custom-js-transitions

If you can update the Toast transition I think this PR is officially ready for the v2 release!

[saturnonearth]

I noted that Toasts still implement duration due the fact it implements the custom crossfade animation by (Cory) @saturnonearth. Unfortunately this is an exception to our new standard, so I'm leaning towards us reverting back to a standard animation in the meantime (there is a canned crossfade transition available).

I agree