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

Implement dynamic animations for all components #1322

Closed
4 tasks
endigo9740 opened this issue Apr 13, 2023 · 13 comments · Fixed by #1533
Closed
4 tasks

Implement dynamic animations for all components #1322

endigo9740 opened this issue Apr 13, 2023 · 13 comments · Fixed by #1533
Assignees
Labels
enhancement New feature or request feature request Request a feature or introduce and update to the project.

Comments

@endigo9740
Copy link
Contributor

endigo9740 commented Apr 13, 2023

Describe what feature you'd like. Pseudo-code, mockups, or screenshots of similar solutions are encouraged!

There will be multiple parts to this process:

  • We need to determine which components use transitions, so we know which we're working with. @JustBarnt create a list and drop it in a comment below so we can review.
  • We then need to update these components to use the new prop approach, preferably using the split in/out transitions like this PR - Add ability to set modal animation duration In/Out seperately #1303
  • We need to mark the old duration props as depreciated. Keep them for backwards compatibility for now, but they will no longer be used. Removing them would be a breaking change.
  • Then we need to test and implement the means make transitions opt-in and interchangeable. My theory is we can do this by allowing users to pass their own transitions (either Svelte-provided or custom), similar to how we allow dynamic actions/actionParams for the Avatar component:
    https://github.com/skeletonlabs/skeleton/blob/dev/src/lib/components/Avatar/Avatar.svelte#L18

Per that last point, I believe transitions are functions, like actions. I also anticipate both the transition and it's params will need to be passed, like so:

  • transitionIn: [transition, {params}] - handles the entry transition and params.
  • transitionOut: [transition, {params}] - handles the exit transition and params.

I'm not opposed to the default transitionX props using the same canned Svelte animations they do currently. This would mean that users get the same "eye candy" out of the box they do now.

I'd expect the default value for the param props would be an object containing only the duration value, as I believe this is the only standard key between all transitions:

export let transitionIn = [slide, { duration: 200 }];

If you have any questions about this please let me know.

What type of pull request would this be?

Enhancement

Any links to similar examples or other references we should review?

Here's how we handle passing an action/actionParams as part of the Avatar component:
https://github.com/skeletonlabs/skeleton/blob/dev/src/lib/components/Avatar/Avatar.svelte#L14

Here's Svelte's tutorial about the split in/out transition attributes:
https://svelte.dev/tutorial/in-and-out

Here's the Svelte tutorial about custom transitions. They are functions, similar to Actions:
https://svelte.dev/tutorial/custom-css-transitions

@endigo9740 endigo9740 added enhancement New feature or request feature request Request a feature or introduce and update to the project. labels Apr 13, 2023
@JustBarnt
Copy link
Contributor

Got it noted to check tonight!

@saturnonearth
Copy link
Contributor

Per that last point, I believe transitions are functions, like actions. I also anticipate the animation and animation parameters will need to be separate props. So all in all we'll have four total props introduced:

transitionIn - the entry animation source
transitionInParams - an object of parameters for the entry animation
transitionOut - the exit animation source
transitionOutParams - an object of parameters for the exit animation

Personally I think having an option for a single value or an array pair works better than separate In/Out prop values. Having separate props for things that should be in pairs will lead to having an exhaustive amount of props on any component that needs fine tune control - akin to Tailwind CSS when you need dark/light/responsive properties, which entire libraries such as WindiCSS tried to fix.

Related values should always stick together, imo.

@endigo9740
Copy link
Contributor Author

endigo9740 commented Apr 13, 2023

I was noodling this exact idea, so I think you've validated that for me. I'll adjust the proposal above to account for this.

Revised as follows:

  • transitionIn: [transition, {params}] - handles the entry transition and params.
  • transitionOut: [transition, {params}] - handles the exit transition and params.

@JustBarnt
Copy link
Contributor

JustBarnt commented Apr 13, 2023

@endigo9740 List of Components & Utilities with what transitions and animations are applied to them currently.

Components

  • AccordionItem
    • Transition: slide
  • Autocomplete
    • Animate: flip
    • Transition: slide
  • InputChip
    • Animate: flip
    • Transition: fly, scale
  • Step
    • Transition: fade
  • Stepper
    • Transition: fade
  • TableOfContents
    • Transition: fade
  • Counter (PR pending)

Utilities

  • Drawer
    • Transition: fade, fly
  • Modal
    • Transition: fade, fly
  • Toast
    • Animate: flip
    • Transition: fly

@saturnonearth
Copy link
Contributor

I was noodling this exact idea, so I think you've validated that for me. I'll adjust the proposal above to account for this.

Revised as follows:

  • transitionIn: [transition, {params}] - handles the entry transition and params.
  • transitionOut: [transition, {params}] - handles the exit transition and params.

image

Imo, I would even go a step further:

transition = {
  in: [transition, {params}],
  out: [transition, {params}]
}

If the user doesn't include one, (...)spread the defaults

And then to apply same transition to both in/out both:

transition = [transition, {params}]

@endigo9740
Copy link
Contributor Author

endigo9740 commented Apr 13, 2023

@JustBarnt thanks, there's quite a few as I suspected. Thank you for detailing which specific transitions are used as well. This does highlight the fact that some components use multiple transitions, so we need to take that into consideration.

@saturnonearth perhaps, but let me think about it a bit longer. I realize it's technically possible, but I could see some users being confused into thinking they always have to supply both in/out, even if we know that's not the case. Remember we're building for a wide audience, and things like this can trip up more folks than you might realize.

@endigo9740
Copy link
Contributor Author

@JustBarnt will not have time to resolve this, so this is open for anyone to jump on! Volunteers welcome!

@endigo9740 endigo9740 unpinned this issue Apr 21, 2023
@endigo9740 endigo9740 added the help wanted Extra attention is needed label Apr 21, 2023
@endigo9740
Copy link
Contributor Author

endigo9740 commented May 4, 2023

FYI I'm going to fold this request into this update:

I'll go ahead and close out the original ticket in favor of handling this here.

@Mahmoud-zino
Copy link
Sponsor Contributor

I would love to work on this issue.

@endigo9740
Copy link
Contributor Author

@Mahmoud-zino I'm typically away on weekends, if you don't mind let's sync on Discord on Monday (US central time) for this. I want to understand you understand the requirements and ensure I'm available if you have questions. Please and thanks!

@Mahmoud-zino
Copy link
Sponsor Contributor

@endigo9740 yeah sure, sounds good.
I already have some questions in this regard so I will write you on Discord on Monday.

@endigo9740
Copy link
Contributor Author

endigo9740 commented May 15, 2023

@Mahmoud-zino To update a couple points from the thread - I think we're keen to follow the pattern established by the action/actionParams props. The complexity here comes from the fact we have to handle both in and out animations.

However, I like @saturnonearth suggestion here for the format:
#1322 (comment)

But rather than trying to bundle everything into a single prop, we implement something like this:

export let transition = [transition, {params}];
export let transitionIn = transition;
export let transitionOut = transition;

I know it's not as compact, but the reason for this is in most cases we match the same transition on in/out already.

We may want to destructure these (the naming is merely a suggestion):

const {tIn, tInParams} = transitionInData;
const {tOut, tOutParams} = transitionOutData;

We will for sure want to ensure we're using the in: and out: directives as shown below:

<div in:tIn={tInParams} out:tOut={tOutParams}>

We'll also want to make sure to we set the same default transitions and param settings to match the current components. So if a component is using a fly for in/out with duration 200, make sure we match that as the defaults.

@endigo9740
Copy link
Contributor Author

@saturnonearth Reminder to revisit this animation when these changes above are implemented:
https://discord.com/channels/1003691521280856084/1108560305056907334

@endigo9740 endigo9740 removed the help wanted Extra attention is needed label May 29, 2023
@endigo9740 endigo9740 pinned this issue May 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request feature request Request a feature or introduce and update to the project.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants