-
Notifications
You must be signed in to change notification settings - Fork 747
screenFooter component #3905
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
base: master
Are you sure you want to change the base?
screenFooter component #3905
Conversation
lidord-wix
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:

| /** | ||
| * The background style of the footer | ||
| */ | ||
| backgroundType?: ScreenFooterBackgrounds | `${ScreenFooterBackgrounds}`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! the type is well defined 😄
Did you understand why we're doing it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've seen it done in other places in ui-lib, I guess it's for letting devs write the literal string or to use an enum which supports changes in the enums value?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
exactly, it also supports auto-complete when using it, which is really nice as a user
| ItemsFit, | ||
| Switch, | ||
| TextField, | ||
| Slider, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Slider doesn't work so good, can you try the slider from Incubator?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it seems much more laggy tbh, do we still prefer it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works better for me with Slider from incubator 🙈
| ? {width: itemWidth, flexShrink: 1, overflow: 'hidden' as const} | ||
| : {width: itemWidth, maxWidth: '100%' as const}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please avoid using "as ..." for resolving typing issues, it kinda hack
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's the alternative for it? couldn't find one but seemed to be used throughout the repo.
perhaps using 'ViewStyle'?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, try to use ViewStyle
most of the time it just shows that we don't use the types properly
packages/react-native-ui-lib/src/components/screenFooter/index.tsx
Outdated
Show resolved
Hide resolved
… basic render function, basic styling
added hideOnScroll to footerScreen.
added safeArea support.
…als, added shadow and dividers support
…er centrization of img component
b2a729d to
7547d9d
Compare
✅ PR Description Validation PassedAll required sections are properly filled out:
Your PR is good for review! 🚀 This validation ensures all sections from the PR template are properly filled. |
|
After making some mess with git history due to the new pre-push check that denied my branch name, I've managed to do it properly. the new commits start from I've left replies to some comments due to things I wasn't sure about. Here's what was added in the recent commits here -
this was quite a lot and I still miss some work on the docs part, but let me know what you think overall. |
lidord-wix
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks really good overall, good job 🙂
I replied and added some small comments, please also see bullet 6 in my last review comment (still looks the same)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's rename, maybe bottomGradient or something?
| /** | ||
| * The background style of the footer | ||
| */ | ||
| backgroundType?: ScreenFooterBackgrounds | `${ScreenFooterBackgrounds}`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
exactly, it also supports auto-complete when using it, which is really nice as a user
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see we still have this one
| if (useSafeArea && Constants.isIphoneX) { | ||
| style.push({paddingBottom: safeAreaBottom}); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thinks it's better to retry
in general we don't support landscape in Wix mobile apps (yet), but since it's also for the community, let's take it into consideration :)
| {alignItems, justifyContent} | ||
| ]; | ||
|
|
||
| if (useSafeArea && Constants.isIphoneX) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but only iPhoneX needs this padding?
| } | ||
| }; | ||
|
|
||
| const renderFooterItems = useMemo(() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to memo if it's not passed as a prop value :)
| ? {width: itemWidth, flexShrink: 1, overflow: 'hidden' as const} | ||
| : {width: itemWidth, maxWidth: '100%' as const}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, try to use ViewStyle
most of the time it just shows that we don't use the types properly
| ItemsFit, | ||
| Switch, | ||
| TextField, | ||
| Slider, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works better for me with Slider from incubator 🙈
| }, [visible, height, animationDuration]); | ||
|
|
||
| // Combine keyboard hoisting + visibility into a single animated style | ||
| const animatedStyle = useAnimatedStyle(() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
works much better with the keyboard accessory view
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