Skip to content
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

feat: added dynamic sizing #1513

Merged
merged 5 commits into from
Sep 10, 2023
Merged

feat: added dynamic sizing #1513

merged 5 commits into from
Sep 10, 2023

Conversation

gorhom
Copy link
Owner

@gorhom gorhom commented Sep 9, 2023

Closes #658, closes #32, closes #1363

Motivation

This PR is a replacement for #1510 by @Eli-Nathan & #1402 by @ororsatti, where the implementation been shifted into the core library allowing the removal of the useBottomSheetDynamicSnapPoints hook.

Users will be able to enable dynamic sizing by only providing a boolean prop into the bottom sheet component enableDynamicSizing. And enable setting a limit for content height by providing a max value with maxDynamicContentSize.

Testing

  • tested with Bottom Sheet View
  • tested with Bottom Sheet Scrollables
yarn add ssh://git@github.com:gorhom/react-native-bottom-sheet#feat/dynamic-sizing

@gorhom gorhom added the v4 Written in Reanimated v2 label Sep 9, 2023
@gorhom gorhom self-assigned this Sep 9, 2023
@gorhom gorhom marked this pull request as ready for review September 9, 2023 11:45
Copy link

@Eli-Nathan Eli-Nathan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks amazing! Nice work!
Does this remove the ability for users to set initial snap points and have CONTENT_HEIGHT as a specific snap point?

@@ -48,7 +48,7 @@ export interface BottomSheetProps
* snapPoints={['%100']}
* @type Array<string | number>
*/
snapPoints: Array<string | number> | SharedValue<Array<string | number>>;
snapPoints?: Array<string | number> | SharedValue<Array<string | number>>;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there anything we can do with the type annotation to tell users that this is only not required when the enableDynamicSizing prop is true?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mmm not sure how could we achieve that, but if customers did not provide snap points and dynamic sizing prop, it will throw an exception https://github.com/gorhom/react-native-bottom-sheet/pull/1513/files#diff-620b7ea703632ea948b10134aaea17e0efd09781b64f587ecab4395812f9a3ccR26

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just in the JSDoc comment above I was thinking. Should show in most IDEs when you hover over the prop or while you're typing

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Eli-Nathan i have added an extra description in the latest commit

@gorhom
Copy link
Owner Author

gorhom commented Sep 9, 2023

Does this remove the ability for users to set initial snap points and have CONTENT_HEIGHT as a specific snap point?

users could provide their own snap points, and when the layout is calculated, the logic will insert the content size in the snap points list and sort it accordingly

@ororsatti
Copy link

ororsatti commented Sep 9, 2023

Tested, looks awesome !

@gorhom gorhom merged commit 7330c7c into master Sep 10, 2023
@gorhom gorhom deleted the feat/dynamic-sizing branch September 10, 2023 08:43
@ororsatti
Copy link

When this is expected to be released?

@younes0
Copy link

younes0 commented Sep 10, 2023

@ororsatti this has been released 10 minutes ago
@gorhom thanks for the awesome work

@gorhom
Copy link
Owner Author

gorhom commented Sep 10, 2023

@Eli-Nathan @ororsatti @younes0 please have a look at the docs before using this prop
https://gorhom.github.io/react-native-bottom-sheet/props#enabledynamicsizing

Setting this prop to true, will result in adding a new snap point to the provided snap points and will be sorted accordingly, and this might effect the indexing, for example, if provided snap points are [100, 1000], and the content size is 500 then the final snap points will be [100, 500, 1000].

@Eli-Nathan
Copy link

Eli-Nathan commented Sep 10, 2023

@gorhom thanks for the quick turn around here!
Can I confirm that it's possible to use enabaleDynamicSizing without passing snapPoints?
Just tested it and it throws an error

Scratch that, had to dump my node_modules and reinstall

@rodolphoasb
Copy link

rodolphoasb commented Sep 11, 2023

It's working perfectly here! Great work on this!

A reminder if you are having trouble with the dynamic sizing, don't use SafeAreaView as the child of BottomSheetModal, the size gets miscalculated sometimes, causing your sheet to not fully open and sometimes it opens and close instantly.

Don't use this:

<BottomSheetModal>
  <BottomSheetView>
    <SafeAreaView>
    </SafeAreaView>
  </BottomSheetView>
</BottomSheetModal>

Use something like this:

<BottomSheetModal>
  <BottomSheetView>
    <View>
    </View>
  </BottomSheetView>
</BottomSheetModal>

@gabrieldonadel
Copy link

enableDynamicSizing does not work for me, my BottomSheet is stuck at 0px height

@sylvain-abadie
Copy link

sylvain-abadie commented Oct 27, 2023

@gabrieldonadel Can you try passing a sharedValue for the contentHeight prop?

const animatedContentHeight = useSharedValue(0);

return ( 
  <BottomSheet
        enableDynamicSizing
        contentHeight={animatedContentHeight} >
        ...
  </BottomSheer>
);

@todorone
Copy link

@gabrieldonadel Can you try passing a sharedValue for the contentHeight prop?

const animatedContentHeight = useSharedValue(0);

return ( 
  <BottomSheet
        enableDynamicSizing
        contentHeight={animatedContentHeight} >
        ...
  </BottomSheer>
);

It works with animatedContentHeight, thanks for the workaround. Can it be fixed though to work properly? @gorhom

@younes0
Copy link

younes0 commented Oct 28, 2023

@rodolphoasb how do you handle safe area in your case ?

@todorone
Copy link

@rodolphoasb how do you handle safe area in your case ?

https://github.com/th3rdwave/react-native-safe-area-context#usesafeareainsets

@younes0
Copy link

younes0 commented Oct 28, 2023

@todorone thanks for the documentation which I'm already aware of but this is not helpful.
never mind, I will play with this and provide a full example unless someone does.

@fendermany
Copy link

fendermany commented Oct 31, 2023

const GorhomBottomModal = React.forwardRef<BottomSheet, GorhomBottomModalProps>((props, ref) => {
  const insets = useSafeAreaInsets();

  const renderBackdrop = useCallback(
    (props: any) => <BottomSheetBackdrop disappearsOnIndex={-1} appearsOnIndex={0} {...props} />,
    [],
  );

  return (
    <BottomSheet
      enablePanDownToClose
      ref={ref}
      // detached={true}

      index={-1}
      handleComponent={CustomHandle}
      backdropComponent={renderBackdrop}
      enableDynamicSizing={true}
      backgroundStyle={styles.backgroundStyle}>
      <BottomSheetScrollView>
        <View style={[styles.container, { paddingBottom: insets.bottom + 10 }, props.style]}>{props.children}</View>
      </BottomSheetScrollView>
    </BottomSheet>
  );
});

working example

@karlkristopher
Copy link

@fendermany I believe this example is only working when including <BottomSheetScrollView>.

const GorhomBottomModal = React.forwardRef<BottomSheet, GorhomBottomModalProps>((props, ref) => {
  const insets = useSafeAreaInsets();

  const renderBackdrop = useCallback(
    (props: any) => <BottomSheetBackdrop disappearsOnIndex={-1} appearsOnIndex={0} {...props} />,
    [],
  );

  return (
    <BottomSheet
      enablePanDownToClose
      ref={ref}
      // detached={true}

      index={-1}
      handleComponent={CustomHandle}
      backdropComponent={renderBackdrop}
      enableDynamicSizing={true}
      backgroundStyle={styles.backgroundStyle}>
      <BottomSheetScrollView>
        <View style={[styles.container, { paddingBottom: insets.bottom + 10 }, props.style]}>{props.children}</View>
      </BottomSheetScrollView>
    </BottomSheet>
  );
});

working example

@badalsaibo
Copy link

badalsaibo commented Nov 2, 2023

Any example with both a fixed view and a scrollable content inside?

<BottomSheetModal>
  <View>
    <Text>Some fixed view</Text>
    <BottomSheetScrollView>
        {/* some scrollable list */}
    </BottomSheetScrollView>
  </View>
</BottomSheetModal>

With enableDynamicSizing and maxDynamicSnapPoint the modal does get adjusted, but the calculation is a bit off. (See video)

Record_2023-11-03-14-16-19.mp4

In the video you can see as I switch the tabs, the content is still scrollable even when the height for the new content is well below the maxDynamicContentSize.

<BottomSheetModal
      ref={bottomSheetModalRef}
      enableDynamicSizing
      maxDynamicContentSize={700}
      handleIndicatorStyle={{ display: 'none' }}
      backdropComponent={CustomBackdrop}
      enableContentPanningGesture={false}>
      <View style={{ flex: 1 }}>
        <BottomSheetView>
        </BottomSheetView>
        <BottomSheetScrollView>
          {/* some scrollable list */}
        </BottomSheetScrollView>
      </View>
</BottomSheetModal>

However using useBottomSheetDynamicSnapPoints and useBottomSheetMaxHeight as proposed by @Eli-Nathan here works fine (see video)

Record_2023-11-03-15-04-26.mp4

As you can see here the new content is being rendered fully without being scrolled.

const initialSnapPoints = useMemo(() => ['CONTENT_HEIGHT'], []);
  const {
    animatedHandleHeight,
    animatedSnapPoints,
    animatedContentHeight,
    handleContentLayout,
  } = useBottomSheetDynamicSnapPoints(initialSnapPoints);

  const useBottomSheetMaxHeight = (maxHeight = '85%'): number => {
    const { height: deviceHeight } = useWindowDimensions();
    const maxHeightDecimal = parseFloat(`${maxHeight}`) / 100.0;
    const { top: topSafeAreaInset } = useSafeAreaInsets();
    return (deviceHeight - topSafeAreaInset) * maxHeightDecimal;
  };

  const maxHeightStyle = useBottomSheetMaxHeight('60%');

...
...
<BottomSheetModal
      ref={bottomSheetModalRef}
      snapPoints={animatedSnapPoints as any}
      handleHeight={animatedHandleHeight}
      contentHeight={animatedContentHeight}
      handleIndicatorStyle={{ display: 'none' }}
      backdropComponent={CustomBackdrop}
      enableContentPanningGesture={false}>
      <View style={{ flex: 1 }} onLayout={handleContentLayout}>
        <BottomSheetView>
        </BottomSheetView>
        <BottomSheetScrollView
        </BottomSheetScrollView>
      </View>
    </BottomSheetModal>

@Eli-Nathan
Copy link

Eli-Nathan commented Nov 3, 2023

@badalsaibo yeah I'm still having to use my max height solution alongside this too although I'm able to use it with the new enableDynamicSizing prop:

const MyBottomSheetModal: FunctionComponent<BottomSheetModalProps> = ({ bottomSheetModalRef, maxHeight }) => {
    const calculatedMaxHeight = useBottomSheetMaxHeight(maxHeight)

    return (
        <BottomSheetModal
            ref={bottomSheetModalRef}
            enableDynamicSizing
            maxDynamicContentSize={calculatedMaxHeight}
            // ... more props
        >
          <BottomSheetScrollView style={{ maxHeight: calculatedMaxHeight }}>{children}</BottomSheetScrollView>
        </BottomSheetModal>
...

useBottomSheetMaHeight hook:

/*
    Designed to take the specified max height (or default 85%)
    and take away top safe area inset
*/
import { useWindowDimensions } from "react-native"
import { useSafeAreaInsets } from "react-native-safe-area-context"
import type { UseBottomSheetMaxHeight } from "./types"

type MaxHeight = `${string}%` | number

const getPercentageMaxHeight = (
    maxHeight: `${string}%`,
    safeMaxheightValue: number
) => {
    return safeMaxheightValue * (parseFloat(maxHeight) / 100)
}

const getNumberMaxHeight = (maxHeight: number, safeMaxheightValue: number) => {
    if (maxHeight > safeMaxheightValue) {
        return safeMaxheightValue
    }
    return maxHeight
}

export const useBottomSheetMaxHeight: UseBottomSheetMaxHeight = (
    maxHeight?: MaxHeight
) => {
    const { height: deviceHeight } = useWindowDimensions()
    const { top: topSafeAreaInset } = useSafeAreaInsets()
    if (!maxHeight) {
        return undefined
    }
    const safeDeviceHeight = deviceHeight - topSafeAreaInset
    const maxHeightIsPercentage = typeof maxHeight === "string"
    const maxHeightNumber = maxHeightIsPercentage
        ? getPercentageMaxHeight(maxHeight as `${string}%`, safeDeviceHeight)
        : getNumberMaxHeight(maxHeight as number, safeDeviceHeight)

    return maxHeightNumber
}

This allows you to pass a specific number as he max height, or a percentage
And safe areas are taken into account 😄

Note that I'm using the react-native-safe-area-context lib as a helper here.

Hope that helps

@badalsaibo
Copy link

I don't know man. For some reason it behaves the way as in the first video even with the new enableDynamicSizing prop and your hook 😅

I have no idea what's going on but I'll be saving a copy of useBottomSheetDynamicSnapPoints hook for now incase it gets deprecated tomorrow!

//#region callbacks
const handleContentSizeChange = useStableCallback(
(contentWidth: number, contentHeight: number) => {
animatedContentHeight.value = contentHeight;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't this mean that I can't have additional content inside my bottom sheet if I'm using a scrollable component? The height of the sheet will be set to the height of the scrollable component instead of using the height of all the content in the sheet..

@spsaucier
Copy link

When will this be added to v5? I have migrated to a high v4 but would like to use enableDynamicSizing in v5 as well.

@audn
Copy link

audn commented Apr 28, 2024

I'm not able to get this working with React Navigation Integration, is there any limits to enableDynamicSizing?

@karlkristopher
Copy link

I'm not able to get this working with React Navigation Integration, is there any limits to enableDynamicSizing?

@audn my experience was that when using a nested navigator, a fixed height for the sheet was necessary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v4 Written in Reanimated v2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[v4] Dynamic height with FlatList not scrolling. Dynamic Bottom Sheet height based on Children height