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

Adding set/remove link methods. #87

Merged
merged 3 commits into from
Dec 11, 2018

Conversation

etoledom
Copy link
Contributor

@etoledom etoledom commented Dec 4, 2018

This PR is one step to implement Links managing in Gutenberg mobile.

The current implementation on RCTAztecView is:

//To set a link for the selected text:
setLink("link-to-add.com")

//To set a link without selecting text, with the given title (or text):
setLink("link-to-add.com", "title")

//To remove a link currently under the cursor (selected or not selected):
removeLink()

To give back the information of the current url for the link was quite challenging. My initial intention was to implement a function like getLinkAttributes(callback) that we could call when the information is needed. But I didn't find a way to make it work (Native UI Components seems to be quite different to plane Native Module Components).

To solve this problem and to be able to give back that information to the JS side, I decided to use the same strategy used on onActiveFormatsChange, but giving more information.

I added a onActiveFormatAttributesChange, with a current implementation with just links.
If you select or navigate the cursor to anywhere with a link format, onActiveFormatAttributesChange will be called with this info:

link: {
    isActive: true
    url: "wp.com", 
}

For any other time, it will be called with:

link: {
    isActive: false 
}

This could be used for formats too and potentially replace onActiveFormatsChange.

@Tug let me know what do you think about it. We can still try to find another way, but I didn't find anyway to use callbacks (or promises) to ask and get info back from native code.

In the branch gutenberg/rnmobile/test-links you can find a testing code, and see how it looks:
WordPress/gutenberg@mobile...rnmobile/test-links

links

@etoledom etoledom self-assigned this Dec 4, 2018
@etoledom etoledom requested a review from Tug December 4, 2018 21:36
@etoledom
Copy link
Contributor Author

etoledom commented Dec 5, 2018

cc @daniloercoli since the Android side is also related.

@@ -12,10 +12,13 @@ @interface RCT_EXTERN_MODULE(RCTAztecViewManager, NSObject)
RCT_EXPORT_VIEW_PROPERTY(onSelectionChange, RCTDirectEventBlock)

RCT_EXPORT_VIEW_PROPERTY(onActiveFormatsChange, RCTBubblingEventBlock)
RCT_EXPORT_VIEW_PROPERTY(onActiveFormatAttributesChange, RCTBubblingEventBlock)

RCT_EXPORT_VIEW_PROPERTY(placeholder, NSString)
RCT_EXPORT_VIEW_PROPERTY(placeholderTextColor, UIColor)

RCT_EXTERN_METHOD(applyFormat:(nonnull NSNumber *)node format:(NSString *)format)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think we could use applyFormat instead of setLink and removeLink to set and remove a link?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh right, applyFormat only accepts a string (the format) as parameter, I guess we need something else for the url 👍

@Tug
Copy link
Contributor

Tug commented Dec 5, 2018

Code and logic looks good to me 👍
However I'm no expert in swift so I think someone else should double check

@etoledom
Copy link
Contributor Author

etoledom commented Dec 5, 2018

Thank you @Tug !

@diegoreymendez could you take a look to the native side please?
Thank you :)

Copy link
Contributor

@diegoreymendez diegoreymendez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just two comments for your consideration.

private var previousContentSize: CGSize = .zero

private lazy var placeholderLabel: UILabel = {
let label = UILabel(frame: .zero)
return label
}()

private let formatStringMap: [FormattingIdentifier: String] = [
.bold: "Bold",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The initial character in the string here is capitalized, but not for the others. I'd suggest to standardize for clarity.

onActiveFormatAttributesChange?(["attributes": attributes])
}

private func formatString(from identifier: FormattingIdentifier) -> String? {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Considering that:

  1. This method is only being used in the compactMap() call above as far as I can tell.
  2. This method is effectively equivalent to formatStringMap[identifier] if we need the format string from any other place in our code.

I'm wondering if we really need this to be a method and not just a closure, such as { formatStringMap[$0] } above (untested but you get the idea).

@diegoreymendez
Copy link
Contributor

By the way, code-wise the native end of it looks clean to me. I don't see anything off about it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants