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

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 need the png in 5 sizes to support devices with different densities
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.
where can I get the other sizes from?
| /** | ||
| * 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?
| 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?
| <Slider | ||
| value={itemWidth} | ||
| minimumValue={50} | ||
| maximumValue={500} |
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 range is too big IMO, let's decrease the max 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.
250 is ok or too high/low?
| ? {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'?
packages/react-native-ui-lib/src/components/screenFooter/index.tsx
Outdated
Show resolved
Hide resolved
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 consider taking all the types to a different file
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.
done, let me know if what i've done was ok.
… 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. |
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