Activity

1. tychota commented on Jan 3, 2019

   

   tychota

   on Jan 3, 2019

   ContributorAuthor

   More actions

   patch-package
   --- a/node_modules/react-native/Libraries/Text/BaseText/RCTBaseTextShadowView.m
   +++ b/node_modules/react-native/Libraries/Text/BaseText/RCTBaseTextShadowView.m
   @@ -73,6 +73,33 @@ static void RCTInlineViewYogaNodeDirtied(YGNodeRef node)
      [super removeReactSubview:subview];
    }
    
   +- (NSString *)text
   +{
   +  NSString *text;
   +  for (RCTShadowView *shadowView in self.reactSubviews) {
   +    // Special Case: RCTRawTextShadowView
   +    if ([shadowView isKindOfClass:[RCTRawTextShadowView class]]) {
   +      RCTRawTextShadowView *rawTextShadowView = (RCTRawTextShadowView *)shadowView;
   +      text = rawTextShadowView.text;
   +      continue;
   +    }
   +  }
   +  return text;
   +}
   +
   +- (void)setText:(NSString *)text
   +{
   +  for (RCTShadowView *shadowView in self.reactSubviews) {
   +    // Special Case: RCTRawTextShadowView
   +    if ([shadowView isKindOfClass:[RCTRawTextShadowView class]]) {
   +      RCTRawTextShadowView *rawTextShadowView = (RCTRawTextShadowView *)shadowView;
   +      rawTextShadowView.text = text;
   +      continue;
   +    }
   +  }
   +  [self dirtyLayout];
   +}
   +
    #pragma mark - attributedString
    
    - (NSAttributedString *)attributedTextWithBaseTextAttributes:(nullable RCTTextAttributes *)baseTextAttributes
   --- a/node_modules/react-native/Libraries/Text/BaseText/RCTBaseTextViewManager.m
   +++ b/node_modules/react-native/Libraries/Text/BaseText/RCTBaseTextViewManager.m
   @@ -26,6 +26,7 @@ RCT_EXPORT_MODULE(RCTBaseText)
    #pragma mark - Text Attributes
    
    // Color
   +RCT_EXPORT_SHADOW_PROPERTY(text, NSString)
    RCT_REMAP_SHADOW_PROPERTY(color, textAttributes.foregroundColor, UIColor)
    RCT_REMAP_SHADOW_PROPERTY(backgroundColor, textAttributes.backgroundColor, UIColor)
    RCT_REMAP_SHADOW_PROPERTY(opacity, textAttributes.opacity, CGFloat)
   --- a/node_modules/react-native/Libraries/Text/Text.js
   +++ b/node_modules/react-native/Libraries/Text/Text.js
   @@ -65,6 +65,7 @@ const viewConfig = {
        minimumFontScale: true,
        textBreakStrategy: true,
        onTextLayout: true,
   +    text: true
      },
      directEventTypes: {
        topTextLayout: {

   makes it works on iOS, provided the props is send in form of string, eg:

   export default class App extends Component<Props, State> {
     textRef = createRef();
     state = {
       progress: new Animated.Value(0)
     };
   
     componentDidMount() {
       this.state.progress.addListener(ev => {
         this.textRef.current &&
           this.textRef.current.setNativeProps({
             text: Math.round(ev.value).toString()
           });
       });
       Animated.timing(this.state.progress, {
         toValue: 100,
         duration: 2000
       }).start();
     }
   
     render() {
       return (
         <Animated.View style={styles.container}>
           <Text style={styles.welcome} ref={this.textRef}>
             -
           </Text>
         </Animated.View>
       );
     }
   }

   

   If I forget the toString, I get:

   

   Questions:

   • @shergin any tips how to use RCTConvert or so to force convert to string ?
   • @shergin what do you think of my current approach

   ---

   Next, Android :)
2. tychota commented on Jan 3, 2019

   

   tychota

   on Jan 3, 2019

   Last edited by tychota

   ContributorAuthor

   More actions

   For Android this seems to work:

   /**
    * Copyright (c) 2015-present, Facebook, Inc.
    * <p>
    * This source code is licensed under the MIT license found in the
    * LICENSE file in the root directory of this source tree.
    */
   
   package com.facebook.react.views.text;
   
   public class ReactTextShadowNode extends ReactBaseTextShadowNode {
   
       // .....
   
       @ReactProp(name = "text")
       public void setText(@Nullable String text) {
           List<ReactShadowNode> childrens = this.getChildrenList();
           if (childrens != null) {
               for (ReactShadowNode child : childrens) {
                   if (child instanceof ReactRawTextShadowNode) {
                       ((ReactRawTextShadowNode) child).setText(text);
                   }
               }
           }
           markUpdated();
       }
   }

   

   👍React with 👍1Reacted by thibautguedou3
3. tychota commented on Jan 3, 2019

   

   tychota

   on Jan 3, 2019

   ContributorAuthor

   More actions

   If the approach is OK, let me know, I will create a PR for further discussion.
   If I don't answer ping me on Twitter (Github notification are hard to grasp): @tychota.
4. shergin commented on Jan 3, 2019

   

   shergin

   on Jan 3, 2019

   Contributor

   More actions

   @tychota Thank you for working on this! 👍

   Your approach is okay, but we can improve this a bit more.

   1. We don't need a getter in ShadowView, let's just remove it.
   2. Instead of mutating nested RawTextShadowViews, we have to introduce a new instance variable and store the value directly inside it. Then we have to use that as a prefix of a full value. Same way as we do in TextInput.

   So, the output of def should be "abcdef". Actually, we probably want to make a case where both the property and nested content are not empty illegal. Adam put a lot of consideration into this topic, so I defer this decision to him. cc @rigdern

   And maybe we want to go with "value" instead of "text". Adam, IIRC we discussed this as well, what do you think?
5. rigdern commented on Jan 3, 2019

   

   rigdern

   on Jan 3, 2019

   Contributor

   More actions

   The problem I worked on is different but close enough that I have an opinion here.

   I would say that it's not a bug that setNativeProps doesn't work on the text prop of a Text component. The Text component doesn't have a text prop. In addition to setNativeProps not doing anything, code like <Text text='foo' /> doesn't do anything. The way to render text is to pass it as children to the Text component: <Text>foo</Text>.

   I just wanted to clarify that I consider our discussion here to be about a new feature rather than a bug fix.

   Adding this new feature will introduce some complexity to the Text component:

   1. Developers will have to decide between passing text as a prop to the Text component (e.g. <Text value='foo' />) and passing text as children to the Text component (e.g. <Text>foo</Text>). Today, developers don't have to give this any thought because the latter is the only option.
   2. We'll have to handle the case that the developer passes Text as both a prop and as children (e.g. <Text value='foo'>bar</Text>). What would a developer expect to happen? Should there be a runtime error? Should one take priority over the other? Should we render them both?
   3. Related to (2), a developer might end up having both the prop and the children set by passing the initial text as children and then using setNativeProps. Example: <Text>bar</Text> and later they do setNativeProps({ value: 'foo' }). We have to consider that our solution to (2) will also determine the behavior of (3).

   @tychota can you describe the real world scenario you are trying to solve and why passing the text as children to the Text component isn't sufficient? Before we decide to add this feature, I want to make sure the benefits we'll get from it are worth the costs (some of which I listed above).

   API Feedback

   If we decide to go forward with this feature, here are some thoughts on the API:

   • I think the prop name should be value instead of text to match TextInput's API. I know that RCTRawText uses the prop name text but I think that RCTRawText is an implementation detail and isn't part of React Native's public API.
   • If you pass text as both a prop and as children to Text, I think we should copy TextInput's behavior. Unfortunately, TextInput does different things depending on the code path you take:
     1. Failed assert (example).
     2. The children are appended to the value prop.

   For Text, I suppose we could do (i) for all code paths but we'll still need to do (ii) in case this happens in a release build where there are no asserts.

   Adam Comella
   Microsoft Corp.
6. tychota commented on Jan 4, 2019

   

   tychota

   on Jan 4, 2019

   Last edited by tychota

   ContributorAuthor

   More actions

   Real Word scenario

   @tychota can you describe the real world scenario you are trying to solve and why passing the text as children to the Text component isn't sufficient? Before we decide to add this feature, I want to make sure the benefits we'll get from it are worth the costs (some of which I listed above).

   Fair point.

   We actually discussed it with @shergin but offline.
   The use case is performance, exactly in the case of what is put on the page https://facebook.github.io/react-native/docs/direct-manipulation

   

   Two examples in my current app:

   Ex1: slider	Ex2: progress

   In both case the performance is slugish. To give you an example with code:

   Progress (example 2)

   1. Unoptimized version

   click to see the code ...

   export class CircleProgression extends PureComponent<IProps, IState> {
     public state: IState = {
       percentage: 0,
     };
   
     private iteration = 0;
   
     private circleDimensions = {
       diameter: 190,
       progressBarWidth: 15,
       get radius() {
         return this.diameter / 2 - this.progressBarWidth / 2;
       },
     };
   
     public componentDidMount() {
       setTimeout(this.animateProgress, 300);
     }
   
     public render() {
       const { chapter } = this.props;
       const { percentage } = this.state;
       const { diameter, radius, progressBarWidth } = this.circleDimensions;
       const perimeter = Math.PI * 2 * radius;
       const offset = perimeter * (1 - percentage / 100);
   
       return (
         <Container>
           <CircleContainer>
             <ProgressionCircleBackground>
               <Svg height={diameter} width={diameter}>
                 <Circle
                   cx={diameter / 2}
                   cy={diameter / 2}
                   r={radius}
                   fill="none"
                   stroke="#ffffff"
                   strokeWidth={progressBarWidth}
                   strokeDasharray={[perimeter]}
                   strokeDashoffset={offset}
                 />
               </Svg>
             </ProgressionCircleBackground>
             <Medallion
               source={this.getMedallionSource(chapter)}
               resizeMode="contain"
             />
           </CircleContainer>
           <ProgressionTextContainer>
             <ProgressionNumber>{Math.floor(percentage)}</ProgressionNumber>
             <ProgressionSymbol>%</ProgressionSymbol>
           </ProgressionTextContainer>
         </Container>
       );
     }
   
     private getMedallionSource = (chapter: TChapterName) => {
       return chapterData[chapter].imageSrc;
     };
   
     private animateProgress = () => {
       const { percentage } = this.state;
       const maxPercentage = chapterData[this.props.chapter].progressionPercentage;
       if (percentage >= maxPercentage) return;
   
       const newProgress = easeInOutExpo(
         this.iteration,
         0,
         maxPercentage / 100,
         70
       );
       this.setState({ percentage: newProgress * 100 });
       this.iteration += 1;
       requestAnimationFrame(this.animateProgress);
     };
   }

   The perf was horrible (drop around 15 fps in debug) still super slugish on release, even on good device (iphoneX and pixel).

   2. Animated svg

   click to see the code ...

   export class CircleProgression extends PureComponent<IProps, IState> {
     public state: IState = {
      percentage: 0,
       animatedPercentage: new Animated.Value(INITIAL_PERCENTAGE),
     };
   
     public componentDidMount() {
       this.listenerId = this.state.animatedPercentage.addListener(e => {
         this.setState({percentage: e.value});
       });
       this.startAnimation();
     }
   
     public get maxPercentage() {
       return chapterData[this.props.chapter].progressionPercentage;
     }
   
     public componentWillUnmount() {
       this.state.animatedPercentage.removeListener(this.listenerId);
     }
   
     public render() {
       const { chapter } = this.props;
       const { diameter, radius, progressBarWidth } = this.circleDimensions;
       const perimeter = Math.PI * 2 * radius;
       const emptyArcSize = perimeter * (1 - this.maxPercentage / 100);
   
       const animatedOffset = this.state.animatedPercentage.interpolate({
         inputRange: [INITIAL_PERCENTAGE, this.maxPercentage],
         outputRange: [perimeter, emptyArcSize],
       });
   
       return (
         <Container>
           <CircleContainer>
             <ProgressionCircleBackground>
               <Svg height={diameter} width={diameter}>
                 <AnimatedCircle
                   cx={diameter / 2}
                   cy={diameter / 2}
                   r={radius}
                   fill="none"
                   stroke="#ffffff"
                   strokeWidth={progressBarWidth}
                   strokeDasharray={[perimeter]}
                   strokeDashoffset={animatedOffset}
                 />
               </Svg>
             </ProgressionCircleBackground>
             <Medallion
               source={this.getMedallionSource(chapter)}
               resizeMode="contain"
             />
           </CircleContainer>
           <ProgressionTextContainer>
             <ProgressionNumber>{Math.floor(this.state.percentage)}</ProgressionNumber>
             <ProgressionSymbol>%</ProgressionSymbol>
           </ProgressionTextContainer>
         </Container>
       );
     }
   
     private startAnimation = () => {
       Animated.timing(this.state.animatedPercentage, {
         toValue: this.maxPercentage,
         delay: ANIMATION_DELAY,
         duration: ANIMATION_DURATION,
         useNativeDriver: true,
       }).start();
     };
   
     private getMedallionSource = (chapter: TChapterName) => {
       return chapterData[chapter].imageSrc;
     };
   }

   We upgraded rn-svg and last version support native driver.
   Perf is way better but still not ok, especially, the text finish animating later than svg

   3. Trick, using Textinput

   click to see the code ...

   export class CircleProgression extends PureComponent<IProps, IState> {
     public state: IState = {
       animatedPercentage: new Animated.Value(INITIAL_PERCENTAGE),
     };
   
     private progression: React.RefObject<TextInput> = React.createRef();
   
     public componentDidMount() {
       this.listenerId = this.state.animatedPercentage.addListener(e => {
         this.updateNativePercentage(e.value);
       });
       this.startAnimation();
     }
   
     public get maxPercentage() {
       return chapterData[this.props.chapter].progressionPercentage;
     }
   
     public componentWillUnmount() {
       this.state.animatedPercentage.removeListener(this.listenerId);
     }
   
     public render() {
       const { chapter } = this.props;
       const { diameter, radius, progressBarWidth } = this.circleDimensions;
       const perimeter = Math.PI * 2 * radius;
       const emptyArcSize = perimeter * (1 - this.maxPercentage / 100);
   
       const animatedOffset = this.state.animatedPercentage.interpolate({
         inputRange: [INITIAL_PERCENTAGE, this.maxPercentage],
         outputRange: [perimeter, emptyArcSize],
       });
   
       return (
         <Container>
           <CircleContainer>
             <ProgressionCircleBackground>
               <Svg height={diameter} width={diameter}>
                 <AnimatedCircle
                   cx={diameter / 2}
                   cy={diameter / 2}
                   r={radius}
                   fill="none"
                   stroke="#ffffff"
                   strokeWidth={progressBarWidth}
                   strokeDasharray={[perimeter]}
                   strokeDashoffset={animatedOffset}
                 />
               </Svg>
             </ProgressionCircleBackground>
             <Medallion
               source={this.getMedallionSource(chapter)}
               resizeMode="contain"
             />
           </CircleContainer>
           <ProgressionNumber ref={this.progression} />
         </Container>
       );
     }
   
     private startAnimation = () => {
       Animated.timing(this.state.animatedPercentage, {
         toValue: this.maxPercentage,
         delay: ANIMATION_DELAY,
         duration: ANIMATION_DURATION,
         useNativeDriver: true,
       }).start();
     };
   
     private updateNativePercentage = (newValue: number) => {
       const currentProgression = this.progression.current;
       if (currentProgression) {
         currentProgression.setNativeProps({
           text: `${Math.floor(newValue)}%`,
         });
       }
     };
   
     private getMedallionSource = (chapter: TChapterName) => {
       return chapterData[chapter].imageSrc;
     };
   }
   
   const ProgressionNumber = styled.TextInput.attrs({
     editable: false,
     defaultValue: `${INITIAL_PERCENTAGE}%`,
   })`
     font-family: ${({ theme }) => theme.fonts.primary.bold};
     color: ${({ theme }) => theme.colors.white};
     font-size: ${({ theme }) => theme.fontSizes.hugeTitle};
   `;

   Consistant frame rate on JS and native thread, max drop to 53fps, super fluid even on low device (galaxy s3).
   But using text input is super hacky.

   Slider (example 1)

   The slider is even worth, user side of view. Since user do a gesture, animation as not only to be fluid but be also in sync. That is impossible to do without nativeDriver (or @kmagiera reanimated).

   You have an example here : https://blog.bam.tech/developper-news/create-vertical-slider-with-react-native-panresponder

   While the animation is cool it does not feel native.

   ---

   Is the example clearer, @rigdern ?
   Do you think it is worth the effort ?
7. tychota commented on Jan 4, 2019

   

   tychota

   on Jan 4, 2019

   Last edited by tychota

   ContributorAuthor

   More actions

   Solutions comparison

   Solution 1: props value or text

   Description, POC: see above

   Note: in comparison to what you said, text seems to work see:

   https://github.com/facebook/react-native/blob/7de2f77d20b600ba30856d75660357ce7577f9ca/Libraries/Text/TextInput/RCTBaseTextInputShadowView.m#L105-L118

   Pro/cons:

   • 👍relatively easy to implement (see POC)
   • 👎pirate props that can make conflicts
   • 👎 a bit hacky (I'm not sure to understand the reason of RawText implementation details)

   Solution 2: textRef.setNativeRef

   Description

   In react Instance, we add setNativeTextand connect it only when the ref is type of Text (no sure how to do).
   That will allow us to do something like this:

   export class CircleProgression extends PureComponent<IProps, IState> {
     public state: IState = {
       animatedPercentage: new Animated.Value(INITIAL_PERCENTAGE),
     };
   
     private progression: React.RefObject<Text> = React.createRef();
   
     public componentDidMount() {
       this.listenerId = this.state.animatedPercentage.addListener(e => {
         this.updateNativePercentage(e.value);
       });
       this.startAnimation();
     }
   
     public render() {
       return (
         <View>
           <Text ref={this.progression} />
         </View>
       );
     }
   
     private startAnimation = () => {
       Animated.timing(this.state.animatedPercentage, {
         toValue: this.maxPercentage,
         delay: ANIMATION_DELAY,
         duration: ANIMATION_DURATION,
         useNativeDriver: true,
       }).start();
     };
   
     private updateNativePercentage = (newValue: number) => {
       const currentProgression = this.progression.current;
       if (currentProgression) {
         // See Here
         // ↓↓↓↓↓↓↓↓
         currentProgression.setNativeText(`${Math.floor(newValue)}%`);
       }
     };
   }

   POC

   ReactNativeFiberHostComponent.prototype.setNativeText = function(nativeText) {
     UIManager.updateNestedView(
       this._nativeTag,
       "RCTRawText",
       { "text": nativeText }
     );
   };

   UIManager.updateNestedView is a new method that loop under the subviews to find matching component.

   Pro/cons:

   • 👍less chance conflict
   • 👍I feel it is more classsy and easily teachable
   • 👍no new props
   • 👎styled-component may need to adapt so we can can call setNativeText in styled.Text
   • 👎more complex to implement (like super complex actually, I think, because we need a way to find the RawText given a Text handle so at least add a method to UIManager : a good method can be UIManager.updateNestedView(parentHandle, childrenClass, props) because it can be used in other cases where a component have an implementation in two native components)

   Solution 3: animate string property

   PR: #18187 (long awaited by community 😍)

   Description

   This allow us to do this:

   export class CircleProgression extends PureComponent<IProps, IState> {
     public state: IState = {
       animatedPercentage: new Animated.Value(INITIAL_PERCENTAGE),
     };
   
     private progression: React.RefObject<Text> = React.createRef();
   
     public componentDidMount() {
       this.startAnimation();
     }
   
     public render() {
       return (
         <Animated.View>
           <Text ref={this.progression}>{this.state.animatedPercentage}</Text>
         </Animated.View>
       );
     }
   
     private startAnimation = () => {
       Animated.timing(this.state.animatedPercentage, {
         toValue: this.maxPercentage,
         delay: ANIMATION_DELAY,
         duration: ANIMATION_DURATION,
         useNativeDriver: true,
       }).start();
     };
   }

   Pro/cons:

   • 👍clean for animated
   • 👍PR is usefull for animated other stuff too, like RNSvg
   • 👍super teachable
   • 👎not working for stuff that does not use animated, like the slider

   Solution 4: reanimated, and react-native-gesture-handler

   by @kmagiera

   Pro/cons:

   • 👍should works for example 1 and ex 2
   • 👍super perf
   • 👎different paradigm, people should use reanimated because they love declarative api, not because animated is limited (in my point of view)
   • 👎 user need to install external stuff
   • 👎we need to still document the limitation of RN and why some props works in direct manipulation and some not (user have no idea about the Text internal details)
8. tychota commented on Jan 4, 2019

   

   tychota

   on Jan 4, 2019

   Last edited by tychota

   ContributorAuthor

   More actions

   @rigdern and @shergin: what do you think ? My recommendation is to do 2 and 3, and edit doc to point to 4.

   @shergin you had concern with 2, what are they ?

   Should I involve other people to the decision ?
9. rigdern commented on Jan 5, 2019

   

   rigdern

   on Jan 5, 2019

   Last edited by rigdern

   Contributor

   More actions

   @tychota thanks for your detailed posts. Understanding the real world problem you are encountering is very helpful.

   Reading thru your two examples, I think you are talking about several different problems:

   1. Frame rate of animating changes to a Text value.
   2. Keeping Text value animation in sync with another animation (e.g. SVG) or a user gesture (e.g. slider).
   3. Frame rate of animating an SVG.
   4. Frame rate of user interacting with a slider.

   For example in "Progress (example 2) -- 2. Animated svg", you mentioned that upgrading rn-svg improved the performance. I suspect that the improvement is all due to (3).

   I think this GitHub issue should focus on (1) and (2).

   Let's talk about what an ideal API might look like. Once that's decided, we can see how to implement it.

   If your problem is all about animating the value of Text, then we'll want to integrate with Animated. Something like this (you mentioned this under "Solution 3: animate string property"):

   (assume that animatedValue is of type Animated.Value or something similar in the examples below)

   // Animated.Value in `Text` children
   //
   
   <Text>
     {animatedValue}
   </Text>
   

   Potential Problem: Are Animated.Values supported as children anywhere in React Native? If not, this option won't work.

   // Animated.Value in `Text's` `value` prop
   //
   
   <Text value={animatedValue} />
   

   Limitation: You can only pass a string to the Text. Inline images/views and formatting of subsets of Text via nested Text are not supported. Is this limitation acceptable?

   @tychota Do you see any problems with the above 2 APIs for solving your problems (let's ignore the implementation details of these APIs for now)? Am I correct in assuming that ideally you want to update the value of the Text via Animated values for both of your scenarios?

   Potential App-Level Workaround

   @tychota Here's a workaround that might work for you today.

   The idea is that the ImperativeText component below minimizes the amount of work that must be done to update the Text value. When you want to update the Text value, instead of dirtying and rerendering all of CircleProgression or whatever, just ImperativeText gets dirtied and needs to rerender.

   I haven't tested this so I don't know if it's fast enough for your scenario.

   class ImperativeText extends Component {
     state = {
       value: this.props.initialValue
     };
   
     setValue(value) {
       this.setState({ value: value });
     }
   
     render() {
       return <Text {...this.props}>{value}</Text>;
     }
   }
   
   // Example usage of `ImperativeText`
   //
   
   // Use `ImperativeText` in your render function
   <ImperativeText ref={this.label} initialValue="0%" />
   
   // ...
   
   // Update `ImperativeText's` value whenever `progress` changes
   this.label.current.setValue(progress + '%');
   

   Note that I modeled ImperativeText after the Fully uncontrolled component with a key pattern to avoid the gotchas described in that blog post.

   Thoughts on @tychota's Solutions

   Solution 1: props value or text

   I suspect that this solution will facilitate a developer passing an Animated.Value to the value/text prop. I really like this aspect. The downsides were covered in my earlier comment.

   Solution 2: textRef.setNativeText

   Unlike "Solution 1", I suspect that this solution won't help us update the Text's value via an Animated value (because Animated leverages setNativeProps and doesn't know about setNativeText). This seems like a big downside.

   Solution 3: animate string property (aka PR #18187)

   Are you sure that PR #18187 alone will enable code like <Text>{this.state.animatedPercentage}</Text> to work? Earlier in this comment, I questioned whether Animated.Values are supported in component children.

   Solution 4: reanimated, and react-native-gesture-handler

   Do these libraries provide a good solution for animating the content of a Text component?
10. rigdern commented on Jan 5, 2019

    

    rigdern

    on Jan 5, 2019

    Contributor

    More actions

    Note: in comparison to what you said, text [on TextInput] seems to work

    I see. The prop is called value on the JavaScript TextInput component but it's called text on the native components that back TextInput.

    I wonder if this causes problems if you try to pass an Animated value to the value prop because Animated might try to call setNativeProps on value rather than text because value is the name of the prop associated with the Animated value.
11. tychota commented on Jan 7, 2019

    

    tychota

    on Jan 7, 2019

    Last edited by tychota

    ContributorAuthor

    More actions

    @tychota thanks for your detailed posts. Understanding the real world problem you are encountering is very helpful.

    Sorry not putting that earlier on.

    Reading thru your two examples, I think you are talking about several different problems:

    I think I have a slightly different classification of the problem. By order of importance

    User problems:

    • the app feel slow / not native

    Dev experience:

    • hard to achieve performance in two cases:
      • animated using a function that depend on time (props = f(time) where f can be easing or interpolation)
      • following a gesture
    • confusing DX where setNativeProps sometimes works, sometimes don't and there is no clue about what works, what don't and what are the limitation

    ---

    Those UX problems and DX problems translate into a few technical problems:

    • animated Text value using Animated in a fast manner that stay in sync (2) compared to other animation and without noticeable performance drop (1)
    • provide a way to direct manipulation for text content so we have consistency for DX and also a way to solve (4) because when the user manipulate a slider, 60 fps is really needed

    Thus we should focus on (1) (2) and (4), in my opinion.

    Let's talk about what an ideal API might look like. Once that's decided, we can see how to implement it.

    If your problem is all about animating the value of Text, then we'll want to integrate with Animated. > Something like this (you mentioned this under "Solution 3: animate string property"):

    (assume that animatedValue is of type Animated.Value or something similar in the examples below)

    // Animated.Value in `Text` children
    //
    
    <Text>
      {animatedValue}
    </Text>
    

    Potential Problem: Are Animated.Values supported as children anywhere in React Native? If not, this option won't work.

    I think it works (for me children is only syntactic sugar for children props) but let me test. If it does not, we should make it works.

    I will try in my Minimal working example.

    Limitation: You can only pass a string to the Text. Inline images/views and formatting of subsets of Text via nested Text are not supported. Is this limitation acceptable?

    I think this works:

    <Text> It is me, <Text style={boldStyle}>Mario</Text></Text>

    was it what you meant ?

    Potential App-Level Workaround

    Sadly it is not enough in both the animated Text (slugish in bad android) and the pan responder slider (not feeling native, even in iPhone X)

    Thoughts on @tychota's Solutions

    Solution 2: seems good concern so I dislike solution 2 a bit more. Plus it require another api to update a props.

    Solution 3: not sure but I can easily (or not) patch the minimal working example here (https://github.com/tychota/bugSetNativePropsText) and see. I will try later this week and report here.

    Solution 4: reanimated, and react-native-gesture-handler
    I think not, good catch: https://github.com/kmagiera/react-native-reanimated/blob/master/src/ConfigHelper.js#L31-L104 :) I will point @kmagiera on this issue and see but I guess for it to support, we should do "solution 1" anyway.
12. rigdern commented on Jan 7, 2019

    

    rigdern

    on Jan 7, 2019

    Contributor

    More actions

    Thus we should focus on (1) (2) and (4), in my opinion.

    I agree with focusing on (1) and (2) in this issue because they are both Text related. If you want to discuss (4) (Frame rate of user interacting with a slider), can you create a different GitHub issue for it? I think it's worth discussing independently in another issue rather than entangling it with the Text conversation here.

    I think it works (for me children is only syntactic sugar for children props) but let me test. If it does not, we should make it works.

    Great! I look forward to hearing about the result of your experiment.

    Limitation: You can only pass a string to the Text. Inline images/views and formatting of subsets of Text via nested Text are not supported. Is this limitation acceptable?

    I think this works:

    <Text> It is me, <Text style={boldStyle}>Mario</Text></Text>
    

    was it what you meant ?

    No. I was trying to convey two different API options for using Animated values to control the Text content and describe the potential limitations of each option. The options were Animated value as children (<Text>{animatedValue}</Text>) and Animated value as a prop (<Text value={animatedValue} />). Only the latter (as a prop) has the limitation that you can't use inline images/views or format subsets of Text via nested Text. Hopefully that clears up what I meant.
13. react-native-bot

    added

    Bug Report

    on Jan 25, 2019
14. hramos

    removed

    Bug Report

    on Feb 6, 2019
15. alloy commented on Mar 19, 2019

    

    alloy

    on Mar 19, 2019

    Contributor

    More actions

    Hello there 👋 this issue seems to have stalled [after some good back and forth!]. I’ll label it as needing Follow Up, meaning that it will get closed in the future after further stagnation.
16. alloy

    added

    Follow Up

    on Mar 19, 2019
17. tychota commented on Mar 19, 2019

    

    tychota

    on Mar 19, 2019

    Last edited by tychota

    ContributorAuthor

    More actions

    Thanks for pinging @alloy, completly forgotten this. You could have closen "stale" it but you chose to ping and that is motivating me to finish that. Thank you :)

    Summary

    To sum stuff up:

    • setNativeProps does not work with text property on Text component #22855 (comment) describes potential solutions: I have 1) working in my project (patch packaged)
    • after discussion here are the final result:
      • solution 1: ok but concerns here: setNativeProps does not work with text property on Text component #22855 (comment) are they show stopper ?
      • solution 2: the more we discuss, the more downside we found, lets say it is ko

    I should test react native Text update for solution 3 but i guess it depends on solution 1 and also #18187 (one year old, @alloy can you ping there tooo so it get merged).

    Maybe someone on rn team have more insight on animated api and how it works and can answer or explain me.

    Next steps

    • Before going i would like an approval of the choice of going for solution 1, with or without renamming value props from "text" to "value". And how to handle concerns.

    • Then I can PR solution 1.

    • Then we can look if solution 3 works out the box, or need another PR (Support Interpolation of strings when using native driver in Animated, fix Expected node to be marked as "native", optimize AnimatedNode creation and connections #18187 or one i can do) to works.

    • Last celebrate :)
18. cpojer

    removed

    Follow Up

    on Mar 26, 2019
19. react-native-bot

    added

    Bug

    on May 4, 2019
20. hramos

    removed

    Type: Bug Report

    on May 6, 2019
21. mayconmesquita commented on Jun 21, 2019

    

    mayconmesquita

    on Jun 21, 2019

    More actions

    This feature would be awesome!! 🥇

    👍React with 👍2Reacted by Lucas Carvalho and barakmichaely
22. stale commented on Sep 19, 2019

    

    staleBot

    on Sep 19, 2019

    More actions

    Hey there, it looks like there has been no activity on this issue recently. Has the issue been fixed, or does it still require the community's attention? This issue may be closed if no further activity occurs. You may also label this issue as a "Discussion" or add it to the "Backlog" and I will leave it open. Thank you for your contributions.
23. stale

    added

    StaleThere has been a lack of activity on this issue and it may be closed soon.There has been a lack of activity on this issue and it may be closed soon.

    on Sep 19, 2019
24. ken0x0a commented on Sep 19, 2019

    

    ken0x0a

    on Sep 19, 2019

    More actions

    Using react-native-reanimated to animate text doesn't work @ android.
    There is a PR for that, but still not merged. 😭
25. stale

    removed

    StaleThere has been a lack of activity on this issue and it may be closed soon.There has been a lack of activity on this issue and it may be closed soon.

    on Sep 19, 2019
26. stale commented on Dec 18, 2019

    

    staleBot

    on Dec 18, 2019

    More actions

    Hey there, it looks like there has been no activity on this issue recently. Has the issue been fixed, or does it still require the community's attention? This issue may be closed if no further activity occurs. You may also label this issue as a "Discussion" or add it to the "Backlog" and I will leave it open. Thank you for your contributions.
27. stale

    added

    StaleThere has been a lack of activity on this issue and it may be closed soon.There has been a lack of activity on this issue and it may be closed soon.

    on Dec 18, 2019
28. stale commented on Dec 25, 2019

    

    staleBot

    on Dec 25, 2019

    More actions

    Closing this issue after a prolonged period of inactivity. If this issue is still present in the latest release, please feel free to create a new issue with up-to-date information.
29. stale

    closed this as completedon Dec 25, 2019
30. react

    locked as resolved and limited conversation to collaborators on Dec 26, 2019