Skip to content

feat: added touch event props#34

Open
neizerth wants to merge 3 commits intoquidone:mainfrom
neizerth:main
Open

feat: added touch event props#34
neizerth wants to merge 3 commits intoquidone:mainfrom
neizerth:main

Conversation

@neizerth
Copy link
Copy Markdown

@neizerth neizerth commented Mar 8, 2025

Hi Sergey!

Thank you for your awesome project!

Added 4 list events:
onTouchStart?: () => void;
onTouchEnd?: () => void;
onTouchCancel?: () => void;
onScrollEnd?: () => void;

I need these props to make possible this behavior: show other values except selected only when touching is enabled

Have a nice day!

Copy link
Copy Markdown
Member

@rozhkovs rozhkovs left a comment

Choose a reason for hiding this comment

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

@neizerth Thank you for the helpful PR! Please, could you address the comments, make adjustments to the code, and overall, this PR will be ready to be closed.)

touching: touching.value,
});

const onTouchStart = useCallback(() => {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I assume that in this part (and the functions below) the useStableCallback hook would be more suitable, because we can always return the same function reference for callback events, which will prevent unnecessary re-renders. Additionally, this will simplify the code.


const onTouchStart = useCallback(() => {
touching.setTrue();
restProps.onTouchStart?.();
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

For onTouchStart and other new events from props, declare them in the same way as for onValueChanging. That is...

onValueChanged,
onValueChanging,
onTouchStart,
...

If a naming conflict arises, you can add the Prop suffix, for example:

{
...
  onScrollEnd: onScrollEndProp,
...
}: ...

This all relates to the destructuring of props.

Copy link
Copy Markdown
Member

@rozhkovs rozhkovs Mar 9, 2025

Choose a reason for hiding this comment

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

@neizerth
Sorry for not providing a good example of what I meant regarding Prop. This postfix needs to be added to variables (if there is a naming conflict) that are extracted from the props variable of a component. A good example is withScrollEndEvent.

image

const onTouchStart = useCallback(() => {
touching.setTrue();
restProps.onTouchStart?.();
}, [touching, restProps]);
Copy link
Copy Markdown
Member

@rozhkovs rozhkovs Mar 9, 2025

Choose a reason for hiding this comment

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

I want to point out that when you pass restProps into dependencies, it will always have a new reference during the parent component's re-render. This will result in a new function reference, leading to further re-renders of child components.

As one option for the future: instead of directly passing restProps, you could use restProps?.onTouchStart. This would also solve the problem.

Please be careful with this next time.)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Yep! You're right! Thank you!

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.

2 participants