Skip to content

Refactor: Upgrade the bottom-sheet package with BottomSheetModal implementation [CRNS-494] - #1073

Merged
vishalnarkhede merged 2 commits into
v4.0.0-beta.2from
khushal87-crns-494
Jan 13, 2022
Merged

vishalnarkhede merged 2 commits into
v4.0.0-beta.2from
khushal87-crns-494

Conversation

@khushal87

@khushal87 khushal87 commented Jan 6, 2022 •

Copy link
Copy Markdown
Contributor

🎯 Goal

The @gorhom/bottom-sheet package was upgraded to v4 and the v3 was deprecated. Hence, this PR focuses on upgrading the package as well as modifying the changes to adapt to the new APi level changes.

Fixes #1053

🛠 Implementation details

The following are the changes introduced in the PR:

  • The package is upgraded to v4.
  • The ImageGrid component's BottomSheet is modified and now uses BottomSheetModal.
  • The snapTo property is changed to snapToIndex.
  • The snap points no more allow 0 as a value so removed it.

🎨 UI Changes

No UI changes as such.

iOS
Before After
Android
Before After

🧪 Testing

☑️ Checklist

  • I have signed the Stream CLA (required)
  • PR targets the develop branch
  • Commits follow the conventional commits spec
  • New code is covered by tests
  • Screenshots added for visual changes
  • Documentation is updated

@khushal87 khushal87 changed the title Refactor: Upgrade the bottom-sheet package with BottomSheetModal implementation Refactor: Upgrade the bottom-sheet package with BottomSheetModal implementation [CRNS-494] Jan 6, 2022
@khushal87
khushal87 marked this pull request as ready for review January 7, 2022 09:44
@madsroskar
madsroskar self-requested a review January 7, 2022 12:20
@madsroskar

Copy link
Copy Markdown
Contributor

I'd suggest squashing the commits here to one, along the lines of

chore: upgrade @gorhom/bottom-sheet to 4.1.5

If we need to look into why the change was made, it'll make it easier to see the whole context in one changeset, since we don't (yet, at least) enforce squash merges

@madsroskar

Copy link
Copy Markdown
Contributor

I'm getting some warnings about unmet peer reqs:

warning "stream-chat-react-native-core > @gorhom/bottom-sheet > @gorhom/portal@1.0.7" has unmet peer dependency "react@*".
warning "stream-chat-react-native-core > @gorhom/bottom-sheet > @gorhom/portal@1.0.7" has unmet peer dependency "react-native@*".
warning "stream-chat-react-native-core > @gorhom/bottom-sheet > react-native-redash@16.2.0" has unmet peer dependency "react@*".
warning "stream-chat-react-native-core > @gorhom/bottom-sheet > react-native-redash@16.2.0" has unmet peer dependency "react-native@*".
warning "stream-chat-react-native-core > @gorhom/bottom-sheet > react-native-redash@16.2.0" has unmet peer dependency "react-native-gesture-handler@*".
warning "stream-chat-react-native-core > @gorhom/bottom-sheet > react-native-redash@16.2.0" has unmet peer dependency "react-native-reanimated@>=2.0.0-rc.0"

I think this is easier to resolve in the context of this change than by itself in the future. react and react-native should be added as devDependencies, redash should probably be a direct dependency for us I think

…odal for ImageGallery

fix: broken tests while using BottomSheetModal component

fix: build fail due to data type in BottomSheetFlatlist

chore: fix dependency issues
Comment on lines +1163 to 1186
<BottomSheetModalProvider>
<BottomSheetModal
animatedIndex={animatedBottomSheetIndex}
enablePanDownToClose={true}
handleComponent={() => (
<ImageGridHandle
closeGridView={closeGridView}
{...imageGalleryCustomComponents?.gridHandle}
/>
)}
handleHeight={imageGalleryGridHandleHeight ?? 40}
index={0}
onChange={(index: number) => setCurrentBottomSheetIndex(index)}
ref={bottomSheetModalRef}
snapPoints={imageGalleryGridSnapPoints || snapPoints}
>
<ImageGrid
closeGridView={closeGridView}
{...imageGalleryCustomComponents?.gridHandle}
numberOfImageGalleryGridColumns={numberOfImageGalleryGridColumns}
photos={photos}
resetVisibleValues={resetVisibleValues}
setImage={setImage}
{...imageGalleryCustomComponents?.grid}
/>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do you think it'd be possible to extract these to a separate smaller component, or do you it'd make things trickier? Would be nice to break down the bigger components we have when we have the chance I think, but there might be things I haven't considered for this case

@khushal87 khushal87 Jan 10, 2022 •

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think we can take up that as a separate PR(if it doesn't seem to be a blocker) because currently it doesn't seem to be doable in one shot and might need some head-up configuring the code. I will create a separate task on JIRA for this if you are fine with it?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Sure, if you think that's the better option, I'm happy to do it that way. I can also share a diff of what I'm thinking of if you'd like/think that might be of value

@vishalnarkhede
vishalnarkhede merged commit 470a600 into v4.0.0-beta.2 Jan 13, 2022
@vishalnarkhede
vishalnarkhede deleted the khushal87-crns-494 branch January 13, 2022 13:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants