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

add scrollTop and scrollLeft bindings #3895

Closed
wants to merge 2 commits into from
Closed

add scrollTop and scrollLeft bindings #3895

wants to merge 2 commits into from

Conversation

Rich-Harris
Copy link
Member

fixes #3780. The binding code gets hairier and hairier... at some point it will probably need a bit of a spring clean

@Rich-Harris
Copy link
Member Author

Actually, I don't think this is quite right. The timeout (which is designed to prevent scroll being set in response to a scroll event) is preventing spring/tweened values from controlling scroll, which isn't what we want. This might also affect <svelte:window>? More work needed

@freedmand
Copy link

What about an approach where you simply set an ignore flag that's cleared in the scroll handler, following https://stackoverflow.com/a/1386750

@Conduitry Conduitry marked this pull request as draft April 29, 2020 21:22
@xylobol
Copy link

xylobol commented May 19, 2021

What about an approach where you simply set an ignore flag that's cleared in the scroll handler, following https://stackoverflow.com/a/1386750

I've implemented something like this for one of my projects. It's determined whether the scroll was user-triggered or script-triggered. We can tell because scripts have to set a variable when trying to scroll.

@stale
Copy link

stale bot commented Dec 15, 2021

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale-bot label Dec 15, 2021
@vercel
Copy link

vercel bot commented Mar 2, 2023

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

Name Status Preview Comments Updated
svelte-dev-2 ❌ Failed (Inspect) Mar 2, 2023 at 11:53AM (UTC)

Copy link

@Imran-imtiaz48 Imran-imtiaz48 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review and Improvement Suggestions:
Error Handling: The refactoring introduces a more consistent approach to error handling by returning errors directly from the function where appropriate. This can improve clarity and maintainability, as each condition now directly returns an error message.

Functionality: Ensure that compiler_errors.invalid_binding(binding.name) correctly generates the error message for invalid bindings. It's important that this function correctly encapsulates the logic for generating error messages based on the binding name.

Readability: Both versions are similar in terms of structure and logic flow, which is good for readability and understanding. Continue to maintain this clarity throughout your codebase.

Testing: After making changes to error handling, it's crucial to test the code to verify that errors are properly thrown and handled under different scenarios.

Documentation: Consider documenting error handling strategies, especially if they involve custom error messages or specific conditions that might not be immediately obvious to others reading the code.

By following these suggestions, you can ensure that your error handling is robust and consistent across your codebase, leading to better maintainability and understanding for future development and maintenance tasks.

@Rich-Harris
Copy link
Member Author

Closing Svelte 4 PR as stale

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.

scrollX/Y for things other than window
5 participants