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

Better support for tablets #216

Closed
eleddie opened this issue Jul 19, 2023 · 4 comments · Fixed by #220
Closed

Better support for tablets #216

eleddie opened this issue Jul 19, 2023 · 4 comments · Fixed by #220
Assignees
Labels
🚀 enhancement New feature or request

Comments

@eleddie
Copy link

eleddie commented Jul 19, 2023

Hi! 👋

Firstly, thanks for your work on this project! 🙂

I was facing an issue where the notifications in tablets are the size of the screen, so they are too big and don't look good.

This is a quick solution I found to have a better UI for notifications on tablets

Here is the diff that solved my problem:

diff --git a/node_modules/react-native-notificated/src/core/config.ts b/node_modules/react-native-notificated/src/core/config.ts
index 954db1c..46f7105 100644
--- a/node_modules/react-native-notificated/src/core/config.ts
+++ b/node_modules/react-native-notificated/src/core/config.ts
@@ -7,6 +7,7 @@ const notificationSideMargin = 14
 const initialOffsetY = -300
 const targetOffsetY = true ? 50 : 10
 const maxNotificationWidth = 343
+const maxNotificationWidthTablet = 420
 
 export const Constants = {
   maxLongPressDragDistance,
@@ -15,4 +16,5 @@ export const Constants = {
   targetOffsetY,
   isAndroid,
   maxNotificationWidth,
+  maxNotificationWidthTablet
 }
diff --git a/node_modules/react-native-notificated/src/core/renderers/GestureHandler.tsx b/node_modules/react-native-notificated/src/core/renderers/GestureHandler.tsx
index 5c67815..20258d4 100644
--- a/node_modules/react-native-notificated/src/core/renderers/GestureHandler.tsx
+++ b/node_modules/react-native-notificated/src/core/renderers/GestureHandler.tsx
@@ -22,8 +22,9 @@ type Props = {
 
 export const GestureHandler = ({ children, state, animationAPI }: Props) => {
   const { width } = useWindowDimensions()
+
   const notificationWidth = state.isPortaitMode
-    ? width - Constants.notificationSideMargin * 2
+    ? Math.min(width, Constants.maxNotificationWidthTablet) - Constants.notificationSideMargin * 2
     : Constants.maxNotificationWidth
   return (
     <PanGestureHandler

This is how it looks with the current version of the library:
Simulator Screen Shot - iPad Pro (12 9-inch) (6th generation) - 2023-07-19 at 18 37 05

It can be fixed by setting maxWidth in the custom notification:
Simulator Screen Shot - iPad Pro (12 9-inch) (6th generation) - 2023-07-19 at 18 33 16

But the problem is that the PanResponder is still full width so you can't tap on the back button when the notification is shown:
Simulator Screen Shot - iPad Pro (12 9-inch) (6th generation) - 2023-07-19 at 18 33 54

And this is how it looks after the change, you can tap on the back button without any issues:
Simulator Screen Shot - iPad Pro (12 9-inch) (6th generation) - 2023-07-19 at 18 32 40

@Okelm
Copy link
Member

Okelm commented Jul 20, 2023

@eleddie thanks for reporting the issue and proposing the solution!

@PdoubleU
Copy link
Collaborator

Hey @eleddie ,

Thanks for giving our library a spin and pointing out this important issue.

Your proposed solution looks really solid. I was wondering if it might be worth adding a new optional configuration element, let's name it notificationWidth, to give developers full control over the width.

Here's what I had in mind:

  • If notificationWidth is left undefined (not set), we'd use your solution.

  • However, a developer might want a notification to span the full width, so they could pass something like useWindowDimensions() to notificationWidth, and use this size regardless of landscape/portrait orientation (the width will be constrained by the screen width).

  • They might also want a specific width for a certain notification or group of notifications, so they'd be able to override this configuration in a narrower scope.

What do you think about this enhancement? Maybe you'd be interested in contributing to our library and preparing a PR with these changes?

Btw, looking at your screenshots, I understood that the library could offer more extensive configuration options for notificationPosition #218

@eleddie
Copy link
Author

eleddie commented Aug 21, 2023

@PdoubleU sounds good! I'll work on it and open a PR once done.
I'll tag you and @Okelm in it👍🏻

@PdoubleU
Copy link
Collaborator

@PdoubleU sounds good! I'll work on it and open a PR once done. I'll tag you and @Okelm in it👍🏻

Hi @eleddie, I just wanted to let you know that we are addressing this issue. Thanks for bringing it to our attention here :)

PdoubleU pushed a commit that referenced this issue Oct 31, 2023
* feature: notification width and position 
* closes #216 && #218
Co-authored-by: PdoubleU <piotrwitasik1988@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🚀 enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants