Add createScrollTo function for scrolling #672
Conversation
🦋 Changeset detectedLatest commit: 5071128 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
Thanks for noticing. I added it and changed "scroll" to "scrolling". |
There was a problem hiding this comment.
I know that the primitive was listed in the ideas to implement. But I wonder whats the actual usecase for this primitive.
- that it works with both signals and element values? I doubt thats usually a problem, and can be solved with
utils.accessor just checking the type. - that it is a noop on the server? since this is an action-something what you call after user interaction or on mount, it shouldn't ever run on a server, and if it does it's a bug and it probably should error then.
- it doesn't really leverage the reactivity in any way
Why yould you use this instead of just calling el.scrollTo, it's not much more code and it will be clearer. in this current form I don't really see it.
Maybe this would make more sense?
function createScrollTo(el: Accessor<Element | Window | null> | Element | Window, options: {
top: MaybeAccessor<number>,
left: MaybeAccessor<number>,
behavor: string,
}): void
let el
createScrollTo(() => el, {top: signal}) // scroll to signal whenever it changes
<div ref={el} />
createScrollTo(signal, {top: 50, behavior: "smooth"}) // scroll to 50 whenever signal changesIt would actually laverage solid effects to call scrollTo when the component is mounted.
|
|
||
| const { left, top, behavior = "auto" } = options; | ||
|
|
||
| if (currentTarget instanceof Window) { |
There was a problem hiding this comment.
Whats this check for? both branches do the same thing
| if (typeof options === "number") { | ||
| options = { | ||
| left: options, | ||
| top: y, | ||
| }; | ||
| } | ||
|
|
||
| const { left, top, behavior = "auto" } = options; | ||
|
|
||
| if (currentTarget instanceof Window) { | ||
| currentTarget.scrollTo({ left, top, behavior }); | ||
| } else { | ||
| currentTarget.scrollTo({ left, top, behavior }); | ||
| } |
There was a problem hiding this comment.
I assume it could be simplified to
currentTarget.scrollTo(typeof options === "number" ? {left: options, top: y} : options)| * scrollTo(100, 200); | ||
| */ | ||
| export function createScrollTo( | ||
| target: Accessor<Element | Window | undefined> | Element | Window = window, |
There was a problem hiding this comment.
default value of window could throw on the server. () => window should work.
| export function createScrollTo( | ||
| target: Accessor<Element | Window | undefined> | Element | Window = window, | ||
| ) { | ||
| return (options: ScrollToOptions | number, y?: number) => { |
There was a problem hiding this comment.
Since this is just returning a callback, it could be simplified to this to avoid currying
function scrollTo(target: Accessor<Element | Window | undefined> | Element | Window, options: ScrollToOptions | number, y?: number): voidthis way it's clear that the utility is not reactive
functions with create__(source: Accessor<T>) signature usually scream that they are reactive
|
Now that I think about it
it says "scroll to a target" not "use |
With options and example