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 useScroll utility hook #9

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

jaydevelopsstuff
Copy link

@jaydevelopsstuff jaydevelopsstuff commented Aug 23, 2024

This PR just adds useScroll as a utility function to get reactive updates from motionone's scroll listener. I tested it in browser and reactivity is working.

I haven't really contributed to or created any TS/Solid libraries before so I am sure there are some things that might need changing. It would be great if someone more experienced could do a proper review to make sure the code is idiomatic and functional.

Relevant issue: #8

@jaydevelopsstuff
Copy link
Author

Any chance someone could review this? It's a pretty minor addition...

Copy link
Member

@thetarnav thetarnav left a comment

Choose a reason for hiding this comment

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

Why stores? Couldn't it be as simple as this?

const [scroll, setScroll] = solid.createSignal<mo.Scroll | null>(null)
solid.onMount(() => mo.scroll(setScroll, options))
return scroll

I'm not really a fan of using stores in libraries.

I don't mind the utility overall though.

The readme should be updated too, so that people can discover it.

@jaydevelopsstuff
Copy link
Author

Why stores? Couldn't it be as simple as this?

const [scroll, setScroll] = solid.createSignal<mo.Scroll | null>(null)
solid.onMount(() => mo.scroll(setScroll, options))
return scroll

I'm not really a fan of using stores in libraries.

I don't mind the utility overall though.

IIRC I tried using just signals originally but couldn't get the reactivity to work. Feel free to play around with it if you would like though.

The readme should be updated too, so that people can discover it.

Sounds good.

@jaydevelopsstuff
Copy link
Author

The reason I didn't use stores is to allow destructuring like this const { time, scrollX, scrollY } = useScroll(), for convenience.

It's been almost a month, any chance this can be merged soon? I need to use it in prod soon.

@strootje
Copy link

strootje commented Oct 18, 2024

I would love to see this merged, currently using a custom hook in my projects that uses signals.

export const useScroll = () => {
  const [time, setTime] = createSignal<number>(0);
  const [scrollX, setScrollX] = createSignal<AxisScrollInfo>(defaultScrollInfo, { equals: false });
  const [scrollY, setScrollY] = createSignal<AxisScrollInfo>(defaultScrollInfo, { equals: false });

  onMount(() => {
    scroll(({ time, x, y }) => {
      setTime(time);
      setScrollX(x);
      setScrollY(y);
    });
  });

  return { time, scrollX, scrollY };
};

The reason for the { equals: false } is needed is because the reference from scroll for the x and y objects are always the same reference. Solid does a === comparison and the references to the objects are always the same.

See the solid docs on signals:
https://docs.solidjs.com/reference/basic-reactivity/create-signal#equals

Feel free to use my implementation.

@@ -65,6 +74,44 @@ export function createMotion(
return state
}

export function useScroll(options?: ScrollOptions): {
time: Accessor<number>
scrollX: AxisScrollInfo

Choose a reason for hiding this comment

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

Suggested change
scrollX: AxisScrollInfo
scrollX: Accessor<AxisScrollInfo>

export function useScroll(options?: ScrollOptions): {
time: Accessor<number>
scrollX: AxisScrollInfo
scrollY: AxisScrollInfo

Choose a reason for hiding this comment

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

Suggested change
scrollY: AxisScrollInfo
scrollY: Accessor<AxisScrollInfo>

scrollY: AxisScrollInfo
} {
const [time, setTime] = createSignal(0)
const [scrollX, setScrollX] = createStore<AxisScrollInfo>({

Choose a reason for hiding this comment

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

Suggested change
const [scrollX, setScrollX] = createStore<AxisScrollInfo>({
const [scrollX, setScrollX] = createSignal<AxisScrollInfo>({

targetLength: 0,
containerLength: 0,
})
const [scrollY, setScrollY] = createStore<AxisScrollInfo>({

Choose a reason for hiding this comment

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

Suggested change
const [scrollY, setScrollY] = createStore<AxisScrollInfo>({
const [scrollY, setScrollY] = createSignal<AxisScrollInfo>({

targetOffset: 0,
targetLength: 0,
containerLength: 0,
})

Choose a reason for hiding this comment

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

Suggested change
})
}, { equals: false })

targetOffset: 0,
targetLength: 0,
containerLength: 0,
})

Choose a reason for hiding this comment

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

Suggested change
})
}, { equals: false })


import {PresenceContext, PresenceContextState} from "./presence.jsx"
import {Options} from "./types.js"
import {createStore, produce} from "solid-js/store"

Choose a reason for hiding this comment

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

Suggested change
import {createStore, produce} from "solid-js/store"

@thetarnav
Copy link
Member

Good to know that the {equals: false} is required for signals to work.
That's a good reason to use stores instead.
Although I don't enjoy that time is an accessor while the other properties are proxies.
a {time: number, x: AxisScrollInfo, y: AxisScrollInfo} interface would work well
exactly what motionone.scroll provides
with a single store instance

const [store, setStore] = createStore({
	time: 0,
	x: {...zero_axis},
	y: {...zero_axis},
})

onMount(() => {
	onCleanup(
		scroll(e => {
			batch(() => {
				setStore('time', e.time)
    			setStore('x', e.x)
    			setStore('y', e.y)
			})
    	}, options)
    )
})

return store

Writing this made me realize that batch and onCleanup were missing

@thetarnav
Copy link
Member

Another option what would keep the same api but not require bringing in stores:

const [time,              set_time]              = createSignal(0)
const [x_current,         set_x_current]         = createSignal(0)
const [y_current,         set_y_current]         = createSignal(0)
const [x_offset,          set_x_offset]          = createSignal([])
const [y_offset,          set_y_offset]          = createSignal([])
const [x_progress,        set_x_progress]        = createSignal(0)
const [y_progress,        set_y_progress]        = createSignal(0)
const [x_scrollLength,    set_x_scrollLength]    = createSignal(0)
const [y_scrollLength,    set_y_scrollLength]    = createSignal(0)
const [x_velocity,        set_x_velocity]        = createSignal(0)
const [y_velocity,        set_y_velocity]        = createSignal(0)
const [x_targetOffset,    set_x_targetOffset]    = createSignal(0)
const [y_targetOffset,    set_y_targetOffset]    = createSignal(0)
const [x_targetLength,    set_x_targetLength]    = createSignal(0)
const [y_targetLength,    set_y_targetLength]    = createSignal(0)
const [x_containerLength, set_x_containerLength] = createSignal(0)
const [y_containerLength, set_y_containerLength] = createSignal(0)

onMount(() => {
	onCleanup(
		scroll(e => {
			batch(() => {
				set_time(e.time)
				set_x_current(e.x.current)
				set_y_current(e.y.current)
				set_x_offset(e.x.offset)
				set_y_offset(e.y.offset)
				set_x_progress(e.x.progress)
				set_y_progress(e.y.progress)
				set_x_scrollLength(e.x.scrollLength)
				set_y_scrollLength(e.y.scrollLength)
				set_x_velocity(e.x.velocity)
				set_y_velocity(e.y.velocity)
				set_x_targetOffset(e.x.targetOffset)
				set_y_targetOffset(e.y.targetOffset)
				set_x_targetLength(e.x.targetLength)
				set_y_targetLength(e.y.targetLength)
				set_x_containerLength(e.x.containerLength)
				set_y_containerLength(e.y.containerLength)
			})
		}, options)
	)
})

return {
	get time()              {return time()},
	x: {
		get current()         {return x_current()},
		get offset()          {return x_offset()},
		get progress()        {return x_progress()},
		get scrollLength()    {return x_scrollLength()},
		get velocity()        {return x_velocity()},
		get targetOffset()    {return x_targetOffset()},
		get targetLength()    {return x_targetLength()},
		get containerLength() {return x_containerLength()},
	},
	y: {
		get current()         {return y_current()},
		get offset()          {return y_offset()},
		get progress()        {return y_progress()},
		get scrollLength()    {return y_scrollLength()},
		get velocity()        {return y_velocity()},
		get targetOffset()    {return y_targetOffset()},
		get targetLength()    {return y_targetLength()},
		get containerLength() {return y_containerLength()},
	},
}

@jaydevelopsstuff
Copy link
Author

Another option what would keep the same api but not require bringing in stores:

const [time,              set_time]              = createSignal(0)

const [x_current,         set_x_current]         = createSignal(0)

const [y_current,         set_y_current]         = createSignal(0)

const [x_offset,          set_x_offset]          = createSignal([])

const [y_offset,          set_y_offset]          = createSignal([])

const [x_progress,        set_x_progress]        = createSignal(0)

const [y_progress,        set_y_progress]        = createSignal(0)

const [x_scrollLength,    set_x_scrollLength]    = createSignal(0)

const [y_scrollLength,    set_y_scrollLength]    = createSignal(0)

const [x_velocity,        set_x_velocity]        = createSignal(0)

const [y_velocity,        set_y_velocity]        = createSignal(0)

const [x_targetOffset,    set_x_targetOffset]    = createSignal(0)

const [y_targetOffset,    set_y_targetOffset]    = createSignal(0)

const [x_targetLength,    set_x_targetLength]    = createSignal(0)

const [y_targetLength,    set_y_targetLength]    = createSignal(0)

const [x_containerLength, set_x_containerLength] = createSignal(0)

const [y_containerLength, set_y_containerLength] = createSignal(0)



onMount(() => {

	onCleanup(

		scroll(e => {

			batch(() => {

				set_time(e.time)

				set_x_current(e.x.current)

				set_y_current(e.y.current)

				set_x_offset(e.x.offset)

				set_y_offset(e.y.offset)

				set_x_progress(e.x.progress)

				set_y_progress(e.y.progress)

				set_x_scrollLength(e.x.scrollLength)

				set_y_scrollLength(e.y.scrollLength)

				set_x_velocity(e.x.velocity)

				set_y_velocity(e.y.velocity)

				set_x_targetOffset(e.x.targetOffset)

				set_y_targetOffset(e.y.targetOffset)

				set_x_targetLength(e.x.targetLength)

				set_y_targetLength(e.y.targetLength)

				set_x_containerLength(e.x.containerLength)

				set_y_containerLength(e.y.containerLength)

			})

		}, options)

	)

})



return {

	get time()              {return time()},

	x: {

		get current()         {return x_current()},

		get offset()          {return x_offset()},

		get progress()        {return x_progress()},

		get scrollLength()    {return x_scrollLength()},

		get velocity()        {return x_velocity()},

		get targetOffset()    {return x_targetOffset()},

		get targetLength()    {return x_targetLength()},

		get containerLength() {return x_containerLength()},

	},

	y: {

		get current()         {return y_current()},

		get offset()          {return y_offset()},

		get progress()        {return y_progress()},

		get scrollLength()    {return y_scrollLength()},

		get velocity()        {return y_velocity()},

		get targetOffset()    {return y_targetOffset()},

		get targetLength()    {return y_targetLength()},

		get containerLength() {return y_containerLength()},

	},

}

Whatever you think is best honestly, you should have commit access for this PR. I just want to get this merged; let me know if there's anything else I can do.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants