shirakaba / react-nativescript

React renderer for NativeScript
https://react-nativescript.netlify.com
MIT License
284 stars 14 forks source link

<$Span> should accept a text node similar to <$Label> #53

Closed spacesuitdiver closed 4 years ago

spacesuitdiver commented 4 years ago

<$Span> should accept a text node similar to <$Label> considering they both use a text prop.

shirakaba commented 4 years ago

Good point! Does $Span render nothing if a text node is provided? I thought the renderer would automatically handle text node addition for any element extending TextBase. But maybe it doesn’t extend TextBase at all.

spacesuitdiver commented 4 years ago

Yea here's the markup vs. rndebugger.

image

image

spacesuitdiver commented 4 years ago

Side note, it's interesting that those <$Label> (or any nested) tags get dumbed down to <class_1> and the text="undefined" prop seems a bit questionable.

Lelelo1 commented 4 years ago

Span is actually inheriting ViewBase, while Label inherits TextBase in nativescript core. RNS Span has mimicked it extending RCTViewBase. It could be more intuitive to be able to put text between the tags on Span as well.

Why Span inherits ViewBase could be due to the ability to display glyphs:

<$Button
    ref={this.button0Ref}
    onTap={() => {
        console.log("hello icon button world)
    }}
 >
    <$FormattedString
        backgroundColor={new Color("transparent")}
    >
        <$Span text={content0.glyph} className={content0.css} />
        <$Span text={this._getText(content0)} />
    </$FormattedString>
</$Button>

Where content0 is instance of:

export class Content {
    text: string;
    css: string;
    glyph: string;
    // onTap: () => void;
}

More on using Span to to have icons: https://stackoverflow.com/questions/57976473/unable-to-use-font-icon-in-nativescript

shirakaba commented 4 years ago

@Lelelo1 Thanks for digging that up. That makes sense.

@spacesuitdiver I've added support for entering text nodes as children into <$Span> elements now, in React NativeScript v0.24.0:

npm install --save react-nativescript

This feature release also removes a redundant call to notifyPropertyChange, which may improve text relayout.


Something related will need revisiting in future, as I've stumbled upon a limitation of React: parent context is only conveyed, in HostConfig.getChildHostContext(), by the name of the primitive element; the Host Config doesn't provide the parent instance itself, so you can't perform instanceof checks at this moment in the rendering lifecycle. Thus, we can't ever say "this child is an ancestor of any arbitrary class extending TextBase".

At the moment, I use this ancestor context to throw an error if ever a user tries to make a text node under the wrong element. But in future, I may have to just allow text nodes anywhere and make the Host Config complain at a later point, e.g. property setting, as in setTextContent().

Until then, non-core components extending TextBase (e.g. there are a few in RadDataForm) or Span will not support child text nodes.

... But one thing at a time!