Tooltip for template selection buttons #2216
Conversation
# Conflicts: # gutenberg
This reverts commit c761dd4.
|
@geriux I had a chance to test over the weekend and it worked as expected in terms of timing and animation. One thing that we discussed earlier is that the colors don't match perfectly with the newish native tooltip component on WPiOS and WPAndroid (which I think is using Color Studio colors), but that I think we should start using system-defined grays on that native component anyway. I just made a proposal for that, and in the process of discussing this, @bjtitus mentioned we may have access to the system-defined colors via a new API (PlatformColor). If that is indeed the case, let's use a system gray and coordinate to decide which colors we should be using for each platform. In the meantime, we can stick with the color you've applied here. Note: we'll also need to create a ticket and discuss/make the same decision on WPAndroid but if we do have access via that API, I suggest we utilize it 😄 |
This reverts commit da1812a.
# Conflicts: # gutenberg # ios/Podfile.lock
There was a problem hiding this comment.
I tested this on a Pixel 3a (real device) via the build apk from the WordPress-Android PR and everything is working as expected. Nice work @geriux .
I understand that it's intentional that this tooltip disappears upon interaction with anything on the page (except scrolling horizontally the template pills), but this behavior is different for the tooltip for "Create a post or page" in the main app screen. That one even survives navigating to the media library, or the post and page lists, and back to the main screen. It only seems to disappear when I tap either the tooltip itself, or the button it's anchored to. Is this difference intentional / expected? cc: @iamthomasbishop
This reverts commit 2ee4845.
# Conflicts: # gutenberg # ios/gutenberg/GutenbergViewController.swift # react-native-gutenberg-bridge/android/src/main/java/org/wordpress/mobile/ReactNativeGutenbergBridge/GutenbergBridgeJS2Parent.java # react-native-gutenberg-bridge/android/src/main/java/org/wordpress/mobile/ReactNativeGutenbergBridge/RNReactNativeGutenbergBridgeModule.java # react-native-gutenberg-bridge/android/src/main/java/org/wordpress/mobile/WPAndroidGlue/WPAndroidGlueCode.java # react-native-gutenberg-bridge/index.js # react-native-gutenberg-bridge/ios/GutenbergBridgeDelegate.swift # react-native-gutenberg-bridge/ios/RNReactNativeGutenbergBridge.m # react-native-gutenberg-bridge/ios/RNReactNativeGutenbergBridge.swift
|
Wanna run full suite of Android and iOS UI tests? Click here and 'Approve' CI job! |
|
@mkevins that’s a good question and a subtle one. The behavior on our side is a bit less aggressive and I think it’s okay. What we have here is feeling pretty natural in this context, so I propose that we keep the behavior as-is. This is a good thing to consider in future definitions — whether they’re persistent or passive (like ours is). |
This reverts commit be9bc7f.
# Conflicts: # gutenberg
|
|
||
| /// Tells the delegate that the editor requested to show the tooltip | ||
| func gutenbergDidRequestStarterPageTemplatesTooltipShown() -> Bool | ||
|
|
||
| /// Tells the delegate that the editor requested to set the tooltip's visibility | ||
| /// - Parameter tooltipShown: Tooltip's visibility value | ||
| func gutenbergDidRequestSetStarterPageTemplatesTooltipShown(_ tooltipShown: Bool) |
There was a problem hiding this comment.
Hey @geriux @chipsnyder !
First congrats for merging this feature 🎉
I just recently came across with these new bridge delegate methods, and I was wondering what was the intention. My first thought was that we were displaying UI on native side, but it seems like it's used only to do persistent storage of user actions (in this case, tooltips already shown to the user). In this case I was wondering if it makes sense to do this persistence internally in the bridge itself, and relieve the client from this responsibility that seems to be quite low level.
Also, it would be interesting to be able to get and set settings in general, in order to avoid new bridge messages for every different setting we might have in the future.
Furthermore, web already has a similar system in place that uses LocalStorage as persistence. It's used (between other things) to know if gutenberg should display NUX UI. Maybe we can reuse this system and create this generic bridge method to store a JSON payload in the same way that the browser's LocalStorage does.
This is clearly no hi-pri, but I've been thinking in ways to improve the bridge usage anyway.
Please let me know what do you think, or perhaps these ideas wouldn't help this feature in particular.
Thank you!
There was a problem hiding this comment.
Hi @etoledom 👋 😃 As I understand, AsyncStorage was considered (to mirror the behavior of LocalStorage), however, it was decided that though that would simplify the implementation of persistent flags, it was more desirable to have all app-level persistence in one place.
While reviewing the Android side of this feature, I had a similar thought about how this pattern might "grow". But, since there aren't any other similar flags (yet) there was nothing to DRY up. My thinking is, we should keep an eye out if a pattern emerges where we can propose a more general solution to settings / persistent flag retrieval and reduce the "footprint" of the interface / protocol.
There was a problem hiding this comment.
Thanks @mkevins for the explanation!
What do you think about the first part? Maybe we still can avoid giving the responsibility to store the flag to the client. Could we do de persistence inside the bridge without the need for WP apps to know about it?
There was a problem hiding this comment.
Maybe we still can avoid giving the responsibility to store the flag to the client. Could we do de persistence inside the bridge without the need for WP apps to know about it?
I think this was the case in an earlier implementation:
https://github.com/WordPress/gutenberg/blob/9aaf218b11481ebb4fe227426921181b21d252b4/packages/block-editor/src/components/page-template-picker/picker.native.js#L94
I'm not sure I have all the context on the decision to centralize the persistence into the parent apps. Perhaps it is to avoid "fragmenting" settings screens, but that is purely a guess on my part. This particular flag is not a user-toggleable setting, so it seems moot in this instance, but maybe this is more of a general architecture decision? I'm not sure there is a way to access the AsyncStorage data from the native side, so perhaps that's a concern as well? Maybe @hypest or @geriux can shed some more light on this.
I found this which looks interesting, but I'm not sure if there is an equivalent iOS module.
There was a problem hiding this comment.
Hey @etoledom 👋 thanks for bringing this up and also thanks @mkevins for joining the conversation.
As @mkevins mentioned when I started this feature my intention was to use AsyncStorage. Since it didn't require much other than adding the library to the main apps (because React Native removed it in favor of lean core).
In the process of that, there were some discussions about maybe avoid using AsyncStorage and use what we currently use for some flags instead. Mainly to store data in the same place, even if it's a flag. So I ended up deciding to do it this way.
For this case though, as @mkevins mentions, it is not a user-togglable setting so having it stored somewhere else would work just fine.
Also, it would be interesting to be able to get and set settings in general, in order to avoid new bridge messages for every different setting we might have in the future.
This could be an option to avoid adding extra code for these types of settings.
Furthermore, web already has a similar system in place that uses LocalStorage as persistence. It's used (between other things) to know if gutenberg should display NUX UI. Maybe we can reuse this system and create this generic bridge method to store a JSON payload in the same way that the browser's LocalStorage does.
I guess this could also work, in my experience, AsyncStorage works just fine. Not sure how complicated it would be to develop what you mention but I'd lean more for adding AsyncStorage if we decide to go for this kind of system.
There was a problem hiding this comment.
Thank you both for your explanations! Everything seems more clear now 🙂
Let's wait for a good moment to re-think this, probably when we need to store a new similar flag.
Fixes #2169
Gutenberg PR-> WordPress/gutenberg#21974WordPress iOS PR-> wordpress-mobile/WordPress-iOS#14045WordPress Android PR-> wordpress-mobile/WordPress-Android#11807To test check the Gutenberg PR description.
Note: Bundles were generated to test the main apps builds, they will be removed before merging.
PR submission checklist:
RELEASE-NOTES.txtif necessary.