react-native icon indicating copy to clipboard operation
react-native copied to clipboard

Initial implementations of spans

Open cubuspl42 opened this issue 1 year ago • 2 comments

Summary:

This is the initial implementation of "spans", the feature tracked in https://github.com/facebook/react-native/issues/42602.

Changelog:

[INTERNAL] [ADDED] - Initial implementation of spans

Test Plan:

cubuspl42 avatar Feb 22 '24 15:02 cubuspl42

@NickGerleman

This PR includes bits based on your feedback from:

  • https://github.com/facebook/react-native/pull/42704
  • https://github.com/facebook/react-native/pull/42701
  1. Would you take a look at these commits...
  • https://github.com/facebook/react-native/pull/43150/commits/1762362e2b6b948d5d474d213a6a584c32d7134f (Introduce SpanFragment and SpanAttributes)
  • https://github.com/facebook/react-native/pull/43150/commits/08ddbff8fd07159ebe2b5f905044c2250a2ab9a5 (Support display: inline style)
  • https://github.com/facebook/react-native/pull/43150/commits/7f2467e14cc969f3fc2d85e6ace65fe588e1806e (Draw spans on Android)

...and give me some high-level feedback? I'm especially interested if there is any chance at all that this direction could lead to the feature being merged.

  1. I know that this PR is big. Do you have any ideas of how this could be split into chunks in a way that any of them would be considered acceptable on its own?

  2. This is a Fabric-only implementation. Would it be acceptable for a new feature to be Fabric-only, or do all features have to support the legacy Paper renderer?

cubuspl42 avatar Feb 22 '24 15:02 cubuspl42

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 17,988,480 +53,419
android hermes armeabi-v7a n/a --
android hermes x86 n/a --
android hermes x86_64 n/a --
android jsc arm64-v8a 21,346,691 +53,407
android jsc armeabi-v7a n/a --
android jsc x86 n/a --
android jsc x86_64 n/a --

Base commit: 767330f21885e668bc0d9b5b3063113d0446bcbc Branch: main

analysis-bot avatar Feb 22 '24 16:02 analysis-bot

@cipolleschi

We had an opportunity to discuss the public API of AttributedString in this PR:

  • https://github.com/facebook/react-native/pull/42596

Would you find a moment to take a look at this commit...

  • https://github.com/facebook/react-native/pull/43150/commits/1762362e2b6b948d5d474d213a6a584c32d7134f (Introduce SpanFragment and SpanAttributes)

...? It significantly changes the API and implementation of AttributedString and its befriended classes. Would such change be considered acceptable when its purpose is implementing a new valuable feature?

cubuspl42 avatar Feb 26 '24 11:02 cubuspl42

Hi @cubuspl42, thanks for the effort! Unfortunately, this PR is too big to be reviewed properly in one go, and it requires different layers of knowledge on the various technologies of the frameworks.

If we want to increase the chances of landing this, I suggest to split it in smaller pieces. Ideally, we can have:

  • a PR for the shared code
  • a PR for iOS
  • a PR for Android
  • a PR for JS.

Also, due to how our systems works, we might have to land the native code some days before the JS code.

cipolleschi avatar Feb 28 '24 11:02 cipolleschi