screenFooter component #3905
screenFooter component #3905
Conversation
… basic render function, basic styling
added hideOnScroll to footerScreen.
added safeArea support.
…als, added shadow and dividers support
lidord-wix
left a comment
lidord-wix
left a comment
There was a problem hiding this comment.
Looks good!
I added some comments, it may look a lot but it's because the PR is a bit big..
Some more general comments:
- Some of the comments I wrote general and not specific only to the place I added them, so please re-review the whole PR when fixing.
- Please run
yarn tscto verify the typescript is ok - Please add api.json file for the docs.
- In general theres no need to pass default values, you get them by default :)
- Please add render tests, you can see references in other components.
- When adding text + image + button, on a stretch case in the example screen, the result is not so good, see the image:
There was a problem hiding this comment.
I think we need the png in 5 sizes to support devices with different densities
| /** | ||
| * The background style of the footer | ||
| */ | ||
| backgroundType?: ScreenFooterBackgrounds | `${ScreenFooterBackgrounds}`; |
There was a problem hiding this comment.
Nice! the type is well defined 😄
Did you understand why we're doing it?
| @@ -0,0 +1,534 @@ | |||
| import React, {useState, useMemo} from 'react'; | |||
| import {StyleSheet, ScrollView, Image} from 'react-native'; | |||
There was a problem hiding this comment.
Please use UILib's Image
| ItemsFit, | ||
| Switch, | ||
| TextField, | ||
| Slider, |
There was a problem hiding this comment.
The Slider doesn't work so good, can you try the slider from Incubator?
| <Slider | ||
| value={itemWidth} | ||
| minimumValue={50} | ||
| maximumValue={500} |
There was a problem hiding this comment.
the range is too big IMO, let's decrease the max value
| ? {width: itemWidth, flexShrink: 1, overflow: 'hidden' as const} | ||
| : {width: itemWidth, maxWidth: '100%' as const}; |
There was a problem hiding this comment.
Please avoid using "as ..." for resolving typing issues, it kinda hack
| background: { | ||
| width: '100%', | ||
| height: '100%' | ||
| } |
There was a problem hiding this comment.
is it needed for non-Image components?
| // Update visibility translation when visible or height changes | ||
| useEffect(() => { | ||
| visibilityTranslateY.value = withTiming(visible ? 0 : height, {duration: animationDuration}); | ||
| }, [visible, height, animationDuration]); |
There was a problem hiding this comment.
Please add visibilityTranslateY to the deps array
| {label: 'Spread', value: HorizontalItemsDistribution.SPREAD} | ||
| ]; | ||
|
|
||
| const DISTRIBUTION_OPTIONS_SPACED = [ |
There was a problem hiding this comment.
Please consider taking all the types to a different file
Description
Added ScreenFooter, a new component for sticky bottom items (3 max) with support for:
Added ScreenFooterScreen demo to showcase all configurations.
Added hook 'useScrollToHide' to allow functionality of 'hideOnScroll'.
Changelog
screenFooter - added new component.
screenFooterScreen - added demo screen for new component.
useScrollToHide - added new hook for controlling visibility during scroll.
Additional info
Ticket 4330