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

fix: send in/out to transition fn #8318

Merged
merged 1 commit into from
Apr 11, 2023

Conversation

tivac
Copy link
Contributor

@tivac tivac commented Feb 24, 2023

After #3918 was fixed by #8068 it was noticed by @robertadamsonsmith that if your transition function is used as a combined in/out transition, it still receives { direction: 'both' } when running the function instead of in or out depending on the direction the transition is going. This severely limits the utility of passing the direction value in the first place.

#3918 (comment)

I agree with him that this is a bug in the original version of #8068, so I'm sending this PR in the hopes that we can get that corrected before too long.

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.

Tests

  • Run the tests with npm test and lint the project with npm run lint

Instead of "both", which doesn't make sense at that point.
@vercel
Copy link

vercel bot commented Feb 24, 2023

@tivac is attempting to deploy a commit to the Svelte Team on Vercel.

A member of the Team first needs to authorize it.

@vercel
Copy link

vercel bot commented Feb 24, 2023

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

Name Status Preview Comments Updated
svelte-dev-2 ❌ Failed (Inspect) Feb 24, 2023 at 3:39PM (UTC)

@tivac
Copy link
Contributor Author

tivac commented Feb 24, 2023

I dunno what to do with this vercel deployment failure, I tried the "inspect" link but I wasn't able to get access to anything to debug it. Can someone maybe re-kick it for me just in case it's a transient issue?

@tivac
Copy link
Contributor Author

tivac commented Feb 27, 2023

I could also update this to add a second parameter like options.current or options.currentDirection so that direction can remain as-is, if it'd help this get merged since it would no longer be a breaking change. 👍🏻

@dummdidumm dummdidumm added this to the 4.x milestone Mar 20, 2023
@dummdidumm dummdidumm changed the base branch from master to version-4 April 11, 2023 13:26
@dummdidumm dummdidumm merged commit 75aec41 into sveltejs:version-4 Apr 11, 2023
@tivac tivac deleted the transition-direction branch April 11, 2023 23:25
dummdidumm pushed a commit that referenced this pull request Apr 18, 2023
Instead of "both", which doesn't make sense at that point.
@gtm-nayan gtm-nayan mentioned this pull request Jun 17, 2023
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants