-
Notifications
You must be signed in to change notification settings - Fork 58
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
FloatingToolbar ideal version #1414
Comments
After spending some time looking at this issue here's what I found so far: iOSiOS is the easier one here. For iOS, you can add a custom view "OverflowView" to the view hierarchy and override Then if you compare that to the normalized frame:
Then you can traverse the view hierarchy and respond appropriately to the touch. The end result looks something like this:
I was also able to get this to work in a less ideal way in iOS with no code changes but by increasing the AndroidAndroid has been a little more problematic. As a refresher for handling touch events in Android, I would recommend looking at
I set up a simplified example of this problem in Expo In the above image, the Red and Orange boxes represent a block, and the Green box is the floating toolbar.
With these changes, if you entered the inspector and tapped on the Floating Toolbar the correct Block would be highlighted but no clicks would process. Using the example above: If I tapped on the Floatingtoolbar (green box) and had break points in I did find this PR facebook/react-native#26374 which seems to address part of our problem. However, trying this locally doesn't seem to fully resolve the issue. I think by overriding ReactViewGroup to parse through the views in an order based on z-index and having the views follow the Android touch detection pattern in that order would allow us to capture the taps and adjust accordingly. Will need to come back to this later |
Do we really need a native change to make it works on iOS? As far as I remember we were able to implement that on iOS and only Android was the problem, but i'm not 100% sure :) |
@dratwas Correct me if I am wrong but I think it was still a kind of workaround. AFAIR, the solution was putting the top most block's FloatingToolbar in the parent level instead of the block level. It worked on iOS but it also introduced an exception in the code design which we don't prefer in the long term. |
@pinarol I think it was the Android case. I created a simple snack with https://snack.expo.io/rJiqFLbXU and it seems to work on iOS. Also, I decided to check that in our app since it could work differently in the snack. To reproduce it please open position:absolute;
top: -50; to the Should look like .floatingToolbar {
background-color: $dark-gray-500;
margin: auto;
min-width: 100;
max-height: $floating-toolbar-height;
border-radius: 22px;
flex-direction: row;
z-index: 100;
height: $floating-toolbar-height;
align-items: center;
justify-content: center;
align-self: center;
margin-bottom: 8px;
position:absolute;
top: -50;
} The floating toolbar is positioned absolute and all is clickable inside w/o any native change. |
@dratwas You're right I had forgotten I got that working in my Snack with no native changes. But I wasn't able to get the correct highlights in Android without doing some increase on the hitslop. Once I started adding the hitslop spacing, it would still work, but I would probably need some native code still. |
In iOS or Android? I agree that we need some native changes on Android but not sure why we need these on iOS. |
Oh sorry yeah I didn't really complete that thought. I meant in both but only if hitslop is required for the end Android Implementation. However, we could only add that if the platform is Android. The issue I was running into with iOS and an increased hitslop then the view below would get the taps on iOS when next to the floating toolbar. That all depends on where Android ends up though. |
I dug into history to be sure. Unfortunately, it wasn't working on iOS without the workaround at RN side. And locate this commit under it: Only after that workaround, we had a working result for iOS. Then there’s a new comment saying that workaround didn’t work for Android: WordPress/gutenberg#17399 (comment) |
I'd like to underline one thing to avoid confusions, the unreceived touch issue on the FloatingBar only manifests itself at the top most block in the FlatList. |
I'll drop a rather more primitive version of the iOS side solution I had in my mind for documentation purposes: Assuming:
on JS side, pass the tag of FloatingToolbar instance explicitly:
on native side, inside OverflowView class, override hitTest and just check for FloatingToolbar intercepts with the touch point:
|
Hey, @chipsnyder I wanted to point to one thing. Please test your solution for the |
@dratwas Good call out. My solution does work for Group Block, which is the primary way I was testing it. |
@chipsnyder could you push your changes to a remote branch and link them here? |
Yup no problem! The work from where I left off is available in these branches: |
As a bit of an update, we are still struggling to get the positioning aspects of the toolbar to cooperate in a scalable way, so per some discussion amongst the team (@pinarol @chipsnyder), we are going take a slightly different approach.
Here is a rough sketch to demonstrate: |
This is the follow-up task for issue: #1413
We should update FloatingToolbar to gain the
Floating
behavior, here's the comparison between temp & ideal solutions:cc: @iamthomasbishop
The text was updated successfully, but these errors were encountered: