feat: Modernize Info Button Bottom Sheets #11282
Conversation
| </FancyModal> | ||
|
|
||
| <AutoHeightBottomSheet visible={modalVisible} onDismiss={() => setModalVisible(false)}> | ||
| <Flex mx={2} mb={4} mt={1} maxHeight={screenHeight - TOP_SCREEN_MARGIN}> |
There was a problem hiding this comment.
Without restricting the height (e.g., with maxHeight), the complete modal can become higher than the screen itself. I plan to move the maxHeight to the AutoHeightBottomSheet because this component seems to be broken.
Also, I'm happy to hear ideas for a better solution without defining something like TOP_SCREEN_MARGIN.
| {!!subTitle && ( | ||
| <Text variant="xs" color="black60"> | ||
| {subTitle} | ||
| </Text> | ||
| )} | ||
| <FancyModal |
There was a problem hiding this comment.
One step closer to deprecate this pretty old component
https://www.notion.so/artsy/Eigen-Deprecate-FancyModal-and-use-Modal-instead-or-Modal-with-presentation-sheet-14ccab0764a08006a0d3e1e3ad5f456c
| import { TouchableOpacity } from "react-native" | ||
|
|
||
| // Necessary screen margin to still see the complete modal on the screen | ||
| const TOP_SCREEN_MARGIN = 120 |
There was a problem hiding this comment.
shouldn't we use safeAreaInsets + some of our spacing as padding instead of this constant margin?
There was a problem hiding this comment.
We also need to keep Font scaling in mind when we use const values
There was a problem hiding this comment.
I found out about the maxDynamicContentSize prop, which works perfectly with our safeAreaInsets.👍
There was a problem hiding this comment.
@dariakoko, thanks for the reminder 🙏 I've checked the modal with increased font size, and in this case, it's no issue because the content is wrapped in a ScrollView.
There was a problem hiding this comment.
Amazing, thank you for checking that!
olerichter00
dismissed stale reviews from dariakoko, araujobarret, and MounirDhahri
via
4c1c6a2
olerichter00
added
the
Squash On Green
olerichter00
force-pushed
the
olerichter00/modernize-info-button-bottom-sheets
branch
from
79d3ded to
d09713e
Compare
artsy-peril
bot
deleted the
olerichter00/modernize-info-button-bottom-sheets
branch
Description
Modernize Info Button Bottom Sheets.
PR Checklist
To the reviewers 👀
Changelog updates
Changelog updates
Cross-platform user-facing changes
iOS user-facing changes
Android user-facing changes
Dev changes
Need help with something? Have a look at our docs, or get in touch with us.