-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
[RNMobile] Add navigation to link settings and add link picker screen to block settings #26206
Conversation
Size Change: 0 B Total Size: 1.19 MB ℹ️ View Unchanged
|
Tested on both platforms and looks properly, didn't notice anything odd. It's a nice feature 🎉 |
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.
Reviewed and tested, and this is looking good. One thing that came to mind while testing for social icons.. do we want to use the link picker for that? At the moment, the link picker suggestions are for the selected site, and so perhaps may not be appropriate for the various social links. Wdyt?
Also, I tried with the image block link settings, but the link picker does not seem to be wired up to the block settings for that block. Is this expected and/or something we will address in a later PR?
4df464c
to
e5f9d5b
Compare
Hmmm, good question. I agree with you that maybe I should create 2 kinds of link-settings - with link picker and w/o link picker. Then we could determine by one prop if we would like to use settings with picker or settings with text input.
The image block has its own picker in edit.native.js file so it doesn't use the same component like a button and social links. I believe it should be changed but in a separate PR. |
This sounds like a good solution.
Ok, thanks for confirming that. 👍 |
e5f9d5b
to
b9f631a
Compare
Hey @mkevins
I already added a prop called I also created an issue to remember that it should be used for the Image block as well and assigned myself. |
7ca7e98
to
7d2b74d
Compare
Hi @dratwas 👋 😄 I tested again with the changes, and it's working as expected with regard to the button and social blocks. Nice work! One thing I noticed, not sure if it's related to this PR, is that the block inserter had some overflow: |
Thanks, @mkevins for the review.
Oh yeah, I created a fix for this here: #26722 Basically, it was the latest change in the previous PR but I didn't notice that the |
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.
Tested again with the lastest change and confirmed it's fixed. Nice work! LGTM!
GB mobile PR: wordpress-mobile/gutenberg-mobile#2742
In this PR i added a link picker screen to the link settings. It is used in Button and Social Icons.
I also added the navigation inside the BottomSheet for LinkSettings with
withBottomSheet
set to true.There is also a prop in LinkSettingsNavigaiton
hasPicker
to determine if we would like to use link picker or regular text inputHow has this been tested?
Case 1:
Case 2:
Screenshots
Types of changes
New Feature
Checklist: