Conversation
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:

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.
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.
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.
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.
you can use modifiers instead this style, also I think you don't need the "row"
There was a problem hiding this comment.
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.
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.
Please see the comments
regarding the build - I see you wasn't on the latest master, I pulled, please re check 🙏🏽
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