Feat/oscillator example #131
Conversation
There was a problem hiding this comment.
Take a look at reanimated slider example: https://docs.swmansion.com/react-native-reanimated/examples/slider/
We can go with similar design to this :)
Besides the comments i've left (which are exaplanatory only), I think that having a constant width slider isn't a problem in our case so we can simplify that part
apps/common-app/src/App.tsx
Outdated
| {Object.entries(Examples).map( | ||
| ([key, example]: [ExampleKey, Example]) => ( | ||
| <TouchableOpacity | ||
| onPress={() => navigation.navigate(key as never)} |
There was a problem hiding this comment.
| onPress={() => navigation.navigate(key as never)} | |
| onPress={() => navigation.navigate(key)} |
| <Text>Oscillator type: {oscillatorType}</Text> | ||
| <Button title={`Click to change type`} onPress={toggleOscillatorType} /> |
There was a problem hiding this comment.
This carousel toggle seems a bit wierd, can we have row of small buttons for each type + highlight for the currently active one?
| const { value, onValueChange, minimumValue, maximumValue, step, style } = | ||
| props; | ||
| const x = useSharedValue(0); | ||
| const [sliderWidth, setSliderWidth] = useState<number>(300); |
There was a problem hiding this comment.
If you want the width to be defined by the user of the component, it should be also used in initialization. Preferably as a separate prop instead of extracting from style prop.
There was a problem hiding this comment.
also it would be better to have sliderWidth as share value, state changes generate component re-renders, usually when handling animations we would like to avoid that as much as possible.
| (xValue: number) => { | ||
| const fraction = xValue / sliderWidth; | ||
| const newValue = minimumValue + fraction * (maximumValue - minimumValue); | ||
| runOnJS(onValueChange)(Math.round(newValue / step) * step); |
There was a problem hiding this comment.
runOnJS call isn't necessary here: convertXToValue is always called inside runOnJS in the gesture callbacks
There was a problem hiding this comment.
Personally I would separate the calculation from setting a value / calling the onValueChange callback
| setSliderWidth(style.width as number); | ||
| } | ||
|
|
||
| runOnJS(convertValueToX)(value); |
There was a problem hiding this comment.
useEffect isn't a reanimated related hook and runs only on the JS thread. Thus we don't need the runONJS function
| runOnJS(convertValueToX)(value); | |
| convertValueToX(value); |
| <View style={[styles.default, style]}> | ||
| <GestureDetector gesture={panGesture}> | ||
| <View | ||
| style={[styles.slider, styles.default, { width: sliderWidth }]} |
There was a problem hiding this comment.
If we want to have variable-size slider, best approach I think would be to:
-
use
sliderWidthas an reanimated shared value -
add an
onLayoutproperty that sets thesliderWidthshared value -
or add ref to the top view component, add
useLayoutEffecthook, that would callmeasurefunction on the view ref and sets up thesliderWidthshared value: https://reactnative.dev/docs/direct-manipulation
| style={[styles.slider, styles.default, { width: sliderWidth }]} | ||
| hitSlop={hitSlop} | ||
| > | ||
| <Animated.View style={[styles.progress, { width: x }]} /> |
There was a problem hiding this comment.
Animation of width is slower than translation! (width changes forces react-native to re-calculate layout of the component)
Instead you can make this view as same width as its container, add a overflow: 'hidden' to the parent and instead of animating the width, animate the translation of this View:
for a value equal to minimum value it would be translated on x by 100% of container width,
for a value equal to max value it would be translated on x by 0.
|
|
||
| useEffect(() => { | ||
| if (style && 'width' in style) { | ||
| setSliderWidth(style.width as number); |
There was a problem hiding this comment.
This won't work, in react native style can be one of 4 types:
- plain js object with styling properties passed directly to the component
style={{ width: 300 }} - style created with
StyleSheet.create({ }), in this case, the the style property will be an integer - id of the registered style. - a boolean :), people often use something like
style={amIInsane && styles.insanityBox} - an array of combination of the 3 above
In order to be able to access property in such mess, you have to call StyleSheet.flatten on the style property first https://reactnative.dev/docs/stylesheet#flatten
But I think in this case I would be misuse of the flatten function
apps/common-app/src/App.tsx
Outdated
| <ScrollView contentContainerStyle={styles.scrollView}> | ||
| {Object.entries(Examples).map(([key, example]) => ( | ||
| <TouchableOpacity | ||
| onPress={() => navigation.navigate(key as never)} | ||
| key={key} | ||
| style={styles.button} | ||
| > | ||
| <Text style={styles.title}>{example.title}</Text> | ||
| <Text style={styles.subtitle}>{example.subtitle}</Text> | ||
| </TouchableOpacity> | ||
| ))} | ||
| {Object.entries(Examples).map( | ||
| ([key, example]: [ExampleKey, Example]) => ( | ||
| <TouchableOpacity | ||
| onPress={() => navigation.navigate(key)} | ||
| key={key} | ||
| style={styles.button} | ||
| > | ||
| <Text style={styles.title}>{example.title}</Text> | ||
| <Text style={styles.subtitle}>{example.subtitle}</Text> | ||
| </TouchableOpacity> | ||
| ) | ||
| )} | ||
| </ScrollView> |
There was a problem hiding this comment.
In case of array of items of same shape etc. it is better to use FlatList component
| oscillatorTypeContainer: { | ||
| flexDirection: 'row', | ||
| marginTop: 20, | ||
| }, |
There was a problem hiding this comment.
oscillatorTypeContainer: {
flexDirection: 'row',
marginTop: 20,
},Not something to fix or change, but small hint, towards such components. Usually I prefer to separate margin around/between components for the components style itself. It helps to reuse same component when there are different margin between components in different places. So for example:
return (
<Screen>
<FirstComponent />
<Spacer.Vertical size={20} />
<Row>
{buttons.map(/* ... *.)}
</Row>
</Screen>
);where spacer is something like:
const Vertical: FC<Props> = ({ size }) => {
return <View style={{ height: size }} />
};
/* ... */
export default {
Vertical,
Horizontal,
}
Closes #127