-
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
Tooltip for template selection buttons #2216
Merged
Merged
Changes from all commits
Commits
Show all changes
41 commits
Select commit
Hold shift + click to select a range
b847aba
Layout picker - Tooltip - Adding AsyncStorage package
96a176f
AsyncStorage - Third party podspec
5473b68
AsyncStorage - Update podspec
c8c2284
Update gutenberg ref
a5ab004
Update release notes
c6d7322
Merge branch 'develop' into feature/layout-picker-tooltip
47cbdfe
Update Gutenberg ref
c761dd4
Bundles for testing
353b737
Update Gutenberg ref
034d6dd
Merge branch 'develop' into feature/layout-picker-tooltip
6a28672
Update Gutenberg ref
4a46bae
Layout picker - Tooltip - Android flag implementation
671e01f
Layout Picker - Tooltip - iOS flag implementation
fbeaab7
Layout picker - Tooltip - iOS Flag implementation typo
6db633c
Layout Picker - Tooltip - Add iOS flag implementation for the demo app
d64a89b
Update Gutenberg ref
32e9c5a
Revert "Bundles for testing"
e32b349
Merge branch 'develop' into feature/layout-picker-tooltip
1d90da0
Fix Release notes after merge
e3bc805
Remove async storage
41b90b1
Remove Async storage for Android
3f38ae4
Update Gutenberg ref
da1812a
Bundles for testing
1dbc6d0
Revert "Bundles for testing"
5fdc010
Merge branch 'develop' into feature/layout-picker-tooltip
ad5bc84
Update Gutenberg ref
2ee4845
Bundles for testing
b67aa13
Revert "Bundles for testing"
8ce3d68
Starter Page Templates - Tooltip - Use request name instead of get
0494fad
Update Gutenberg ref
a37ef95
Merge branch 'develop' into feature/layout-picker-tooltip
c7cb63b
Update Gutenberg ref
706a2fe
Merge branch 'develop' into feature/layout-picker-tooltip
be9bc7f
Bundles for testing
6501269
Update Gutenberg ref
426021f
Update Gutenberg ref
34a1112
Revert "Bundles for testing"
11e65da
Merge branch 'develop' into feature/layout-picker-tooltip
2f76ad1
Update Gutenberg ref
f612740
Update Release Notes
0989b9e
Update Gutenberg ref
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Submodule gutenberg
updated
76 files
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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'sLocalStorage
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
This could be an option to avoid adding extra code for these types of settings.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.