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

fix: replaces useDropdowns by useClickOutside #883

Merged

Conversation

sakul-budhathoki
Copy link
Member

Problem:
Due to the use of Pressable on DropdownsContextProvider, it blocks scroll of ScrollView on mobile app (Android & iOS)

Solution:
Replace useDropdowns by react-native-click-outside

Copy link

netlify bot commented Jan 23, 2024

Deploy Preview for testitori ready!

Name Link
🔨 Latest commit abb50f8
🔍 Latest deploy log https://app.netlify.com/sites/testitori/deploys/65bd21f9593ae30008c8426f
😎 Deploy Preview https://deploy-preview-883--testitori.netlify.app/
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

netlify bot commented Jan 23, 2024

Deploy Preview for teritori-dapp ready!

Name Link
🔨 Latest commit abb50f8
🔍 Latest deploy log https://app.netlify.com/sites/teritori-dapp/deploys/65bd21f968f644000888cb1e
😎 Deploy Preview https://deploy-preview-883--teritori-dapp.netlify.app/
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

App.tsx Outdated
@@ -114,7 +114,7 @@ export default function App() {
</SearchBarContextProvider>
</WalletControlContextProvider>
</WalletsProvider>
</DropdownsContextProvider>
</ClickOutsideProvider>
Copy link
Collaborator

Choose a reason for hiding this comment

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

How about "DropdownsClickOutsideProvider"; maybe is too long, but I feel that with the new name the original intent "Dropdown" got lost.

Copy link
Member Author

@sakul-budhathoki sakul-budhathoki Jan 23, 2024

Choose a reason for hiding this comment

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

Thanks for the idea, yes I agree we can achieve new changes without renames.
I can use:
ClickOutsideProvider -> DropdownsProvider
useClickOutside -> useDropdowns

So, we will have naming conventions as before, and it will be easier for devs to use this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yep totally agree with that too 👍

Copy link
Collaborator

@clegirar clegirar left a comment

Choose a reason for hiding this comment

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

Else found that good 👍 Thanks for that 👍

@@ -118,10 +114,10 @@ export const NFTView: React.FC<{

const NFTViewContent: React.FC<{
nft: NFT;
dropdownRef: RefObject<View>;

Copy link
Collaborator

Choose a reason for hiding this comment

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

This line is empty right ? If you can delete it 👍

@sakul-budhathoki sakul-budhathoki force-pushed the refactor/replace-dropdowns-with-react-native-click-outside branch from d255348 to c008a09 Compare January 23, 2024 15:31
@sakul-budhathoki sakul-budhathoki force-pushed the refactor/replace-dropdowns-with-react-native-click-outside branch from c008a09 to e108ed3 Compare February 1, 2024 03:55
@n0izn0iz n0izn0iz changed the title refactor: replaces useDropdowns by useClickOutside fix: replaces useDropdowns by useClickOutside Feb 2, 2024
@n0izn0iz n0izn0iz merged commit 07acc65 into main Feb 2, 2024
18 checks passed
@n0izn0iz n0izn0iz deleted the refactor/replace-dropdowns-with-react-native-click-outside branch February 2, 2024 17:53
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.

4 participants