-
Notifications
You must be signed in to change notification settings - Fork 751
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:

packages/react-native-ui-lib/src/assets/internal/images/bottomGradient.png
Show resolved
Hide resolved
packages/react-native-ui-lib/src/components/screenFooter/index.tsx
Outdated
Show resolved
Hide resolved
packages/react-native-ui-lib/src/components/screenFooter/index.tsx
Outdated
Show resolved
Hide resolved
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)
packages/react-native-ui-lib/src/assets/internal/images/buttonFloatingOverlay.png
Outdated
Show resolved
Hide resolved
packages/react-native-ui-lib/src/components/screenFooter/index.tsx
Outdated
Show resolved
Hide resolved
packages/react-native-ui-lib/src/assets/internal/images/bottomGradient.png
Show resolved
Hide resolved
packages/react-native-ui-lib/src/components/screenFooter/index.tsx
Outdated
Show resolved
Hide resolved
packages/react-native-ui-lib/src/components/screenFooter/index.tsx
Outdated
Show resolved
Hide resolved
packages/react-native-ui-lib/src/components/screenFooter/index.tsx
Outdated
Show resolved
Hide resolved
packages/react-native-ui-lib/src/components/screenFooter/index.tsx
Outdated
Show resolved
Hide resolved
| const flexStyle = itemsFit === ItemsFit.STRETCH ? {flex: 1, alignItems: 'center'} : {flexShrink: 1}; | ||
| const flexStyle = itemsFit === ItemsFit.STRETCH | ||
| ? {flex: 1, minWidth: 0, alignItems: 'center'} | ||
| : {flexShrink: 1}; |
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.
adding a minWidth prop here to the stretched layout improved the correct division of the container between the items, seems like the button having a certain minWidth caused it to catch more unwanted space
…entering on 'stretch' and 'fixed'
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! 🎉 👏🏽
Approved with a small comment, feel free to merge once fixed :)
| stretchItemWrapper: { | ||
| flexDirection: 'row', | ||
| flex: 1, | ||
| justifyContent: 'center' | ||
| }, |
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.
you can use modifiers instead this style, also I think you don't need the "row"
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's actually necessary - the stretch happens due to the default alignItems: 'stretch', and to not ruin this stretch I needed to kind of cheat it by combining row+justifyContent: center, which caused effect of alignItems: center without overriding the alignItems: stretch.
let me know if I'm missing something though.
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.
Oh, ok 👍🏽
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 still I think you can move to modifiers :)
|
@lidord-wix also, my build fails for some reason due to it not finding the |
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.
Please see the comments
regarding the build - I see you wasn't on the latest master, I pulled, please re check 🙏🏽
| @@ -1,12 +1,14 @@ | |||
| import React, {useCallback, useEffect, useMemo, useState} from 'react'; | |||
| import {LayoutChangeEvent, StyleSheet, ViewStyle} from 'react-native'; | |||
| import {useSafeAreaInsets} from 'react-native-safe-area-context'; | |||
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 see the usage of useSafeAreaInsets in the tabController component, when using a native dependency in one of our components, we're doing it optionally for the users, so if they don't use this component they don't have to install it
| ]; | ||
|
|
||
|
|
||
| if (useSafeArea && !isKeyboardVisible) { |
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 think we don't need the useSafeArea prop anymore, the native control the heights

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