Support textDecorationLine on <Text> components (#709) by ymusiychuk-lohika · Pull Request #1056 · microsoft/react-native-windows

ymusiychuk-lohika · 2017-03-07T16:10:41Z

WPF supports both Underline and Strike-through
UWP supports only Underline. The idea behind this approach was to add an Underline directly to a Paragraph of RichTextBlock, because we know that there is always one Paragraph and thus there should be at most one Underline.

rozele · 2017-03-08T02:26:13Z

ReactWindows/ReactNative.Shared/Views/Text/TextDecorationLine.cs

+        /// Text is both Underlined and Stroke out.
+        /// </summary>
+        [EnumMember(Value = "underline line-through")]
+        UnderlineLineThrough


This is cool, this is compatible with the existing EnumHelpers class? We should re-write all the other enums to use this where possible. Ideally we could get rid of that reflection hack.

We actually don't need this enum, you can just as easily switch case on the string itself.

In reply to: 104831419 [](ancestors = 104831419)

That will avoid the dependency on System.Runtime.Serialization, and avoid the reflection driven changes to EnumHelpers

In reply to: 104832645 [](ancestors = 104832645,104831419)

I personally vote for enum instead of string, if there is a limited number of variants, because it
makes corresponding code strongly-typed.

While I tend to agree that strong typing is usually good, I don't like that we have to add yet another dependency and pay an attribute reflection penalty for all enums across the framework just to support this strong typing.

In reply to: 105130501 [](ancestors = 105130501)

When making decision on changing EnumHelpers I was looking onto "textAlign". Logically it is same as "textDecorationLine" and they both should be treated in same way. But "textDecorationLine" contains single value with space in it, which makes it impossible to use existing approach. So, either it must be an exceptional enum, treated differently (How many such enums can we possibly have?) or the approach should be changed. I've chosen the latter.
I am not against "switch" so if you insist, that's ok. But, please note, Reflection does not have "that" bad impact on performance if it happens once. Yes, if we have 100 000 of enums, then we will see the difference, but currently we have ~20 total and maybe it will grow up to 100 or 200 at most.

rozele · 2017-03-08T02:28:30Z

ReactWindows/ReactNative.Shared/Reflection/EnumHelpers.cs

+                var enumMemberAttribute = type.GetField(name).GetCustomAttributes(typeof(EnumMemberAttribute), true).SingleOrDefault();
+                if (enumMemberAttribute != null)
+                {
+                    result.Add(Normalize(((EnumMemberAttribute)enumMemberAttribute).Value), value);


EnumMemberAttribute [](start = 43, length = 19)

Just curious, does the serialization behavior of EnumMemberAttribute only work in one direction? I.e., is the value only used to serialize to string, or is there some tool that can be used for parsing as well?

It works both ways. For example,
Newtonsoft.Json.JsonConvert.SerializeObject(TextDecorationLine.UnderlineLineThrough, new StringEnumConverter())
will respect the EnumMemberAttribute (notice the StringEnumConverter. Without it, enum is serialized as int)

rozele · 2017-03-08T02:29:29Z

ReactWindows/ReactNative.Shared/Reflection/EnumHelpers.cs

+                result.Add(Normalize(name), value);
+
+                var enumMemberAttribute = type.GetField(name).GetCustomAttributes(typeof(EnumMemberAttribute), true).SingleOrDefault();
+                if (enumMemberAttribute != null)


It's unfortunate that we now have to pay a reflection penalty for all other enums where this does not exist. I know this only occurs once, but its a potential performance hit nonetheless.

That's true that Reflection brings in an overhead, but in this case, due to the fact that this method is called only once per enum, I think it's too small to have a significant impact on performance.

rozele · 2017-03-08T02:30:40Z

ReactWindows/ReactNative/Views/Text/ReactTextViewManager.cs

+        /// </summary>
+        /// /// <param name="view">The view.</param>
+        /// <param name="textDecorationLineValue">The TextDecorationLine value.</param>
+        [ReactProp(ViewProps.TextDecorationLine)]


SetTextDecorationLine [](start = 20, length = 21)

I think it would be better to ignore this prop for now than to provide an incomplete implementation. The textDecorationLine should be applicable to virtual text nodes (i.e., those that correspond to the inline instances as well).

Rolled back

rozele · 2017-03-08T02:31:28Z

ReactWindows/ReactNative.Net46/Views/Text/ReactTextShadowNode.cs


+        /// <summary>
+        /// Sets the TextDecorationLine for the node.
+        /// </summary>


TextDecorationLine [](start = 54, length = 18)

autodocs do not have great grammar.

This is a case where I wish we could leave the tag empty since the argument name is self-documenting.

Agree with @matthargett. I've just tried to have autodocs style seem like other similar properties, like FontStyle and LineHeight

rozele · 2017-03-08T02:34:31Z

ReactWindows/ReactNative.Net46/Views/Text/ReactTextShadowNode.cs

            textBlock.FontStyle = _fontStyle ?? new FontStyle();
            textBlock.FontWeight = _fontWeight ?? FontWeights.Normal;
+            textBlock.TextDecorations = new TextDecorationCollection(TextDecorationCollection);



Again, this only solves the problem at the root level of the text node. React Native supports text decoration on the inlines / virtual text nodes.

I can be wrong, but from what I see, node is the lowest-level possible for creating styled text components. I cannot see a way to apply a style to a raw text. It would be great if you can come up with any example to understand the problem.

This would be supported on Android and iOS:

<Text> Here is some regular text <Text style={{textDecorationLine: 'underline'}}>and here is some underlined virtual text</Text>. </Text>

However, this scenario would not supported by this PR.

Nice one. Didn't even got in mind to try this. I'll check how can I implement it this way

My guess would be you may need to create virtual node types and control all this by checking for presence of the prop in JavaScript. I.e., in JavaScript, you might check:

if (this.props.textDecorationLine === 'underline') { this.props.children = (<UnderlinedText>{this.props.children}</UnderlinedText>); }

or something along these lines. I'm sure my syntax is a bit off.

rozele · 2017-03-08T02:37:19Z

🕐

This reverts commit 5de9c8e. # Conflicts: # ReactWindows/ReactNative/Views/Text/ReactTextViewManager.cs

rozele · 2017-03-09T20:47:00Z

Closing and reopening to trigger CLA bot

msftclas · 2017-03-09T20:47:05Z

@ymusiychuk-lohika,
Thanks for having already signed the Contribution License Agreement. Your agreement was validated by Microsoft. We will now review your pull request.
Thanks,
Microsoft Pull Request Bot

…anager, support decoration of inlines

ymusiychuk-lohika · 2017-03-13T14:01:17Z

I've put TextDecorationLine logic into ReactTextViewManager and ReactSpanViewManager. Now it supports inline decoration.
To add, I don't like all the #if WINDOWS_UWP stuff. I think, it should be changed by, maybe, creating base abstract classes in ReactNative.Shared and inheriting them in UWP and Net46 separately. But, most likely it should be done as a seprate task/PR.

codecov-io · 2017-04-07T01:26:47Z

Codecov Report

Merging #1056 into master will decrease coverage by 0.06%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master    #1056      +/-   ##
==========================================
- Coverage   31.68%   31.62%   -0.07%     
==========================================
  Files         253      253              
  Lines       17976    18013      +37     
  Branches     1507     1513       +6     
==========================================
  Hits         5696     5696              
- Misses      12126    12163      +37     
  Partials      154      154

Impacted Files	Coverage Δ
...actNative.Net46/Views/Text/ReactTextViewManager.cs	`0% <0%> (ø)`	⬆️
...ndows/ReactNative.Shared/Reflection/EnumHelpers.cs	`0% <0%> (ø)`	⬆️
...ctNative.Shared/Views/Text/ReactSpanViewManager.cs	`0% <0%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2b81a76...4850e9c. Read the comment docs.

ymusiychuk-lohika and others added 8 commits February 28, 2017 16:30

feat(WPF) Support textDecorationLine on <Text> components (#709)

12ac2cb

Underline implementation for UWP

5de9c8e

Removed reference to System.Runtime.Serialization

b4a8fe2

Styling fixes

bdc0a98

Returned back System.Serialization

c4b93d3

Style changes

27f7ce1

Removed ChakraCore directory

fdbb0ec

Merge branch 'master' into textDecorationLine-support-709

c05a80e

rozele reviewed Mar 8, 2017

View reviewed changes

matthargett and others added 4 commits March 7, 2017 19:16

Merge branch 'master' into textDecorationLine-support-709

5b5aa6b

Small EnumHelpers reflection improvement

355182a

Revert "Underline implementation for UWP"

f35c67a

This reverts commit 5de9c8e. # Conflicts: # ReactWindows/ReactNative/Views/Text/ReactTextViewManager.cs

Line endings fix

ced5687

rozele closed this Mar 9, 2017

rozele reopened this Mar 9, 2017

msftclas added the cla-already-signed label Mar 9, 2017

matthargett and others added 2 commits March 9, 2017 13:36

Merge branch 'master' into textDecorationLine-support-709

1db84ac

Moved implementation from ShadowNode to TextViewManager and SpanViewM…

5e842a0

…anager, support decoration of inlines

Returned ReactTextShadowNode.cs to initial state

e259858

kevinvangelder approved these changes Apr 6, 2017

View reviewed changes

Merge branch 'master' into textDecorationLine-support-709

2e2f60e

Merge branch 'master' into textDecorationLine-support-709

4850e9c

matthargett approved these changes Apr 7, 2017

View reviewed changes

matthargett merged commit 22083ae into microsoft:master Apr 7, 2017

matthargett deleted the textDecorationLine-support-709 branch April 7, 2017 01:34

snyk-bot mentioned this pull request Jan 12, 2022

[Snyk] Fix for 1 vulnerabilities harunpehlivan/react-native-windows#16

Open

Conversation

ymusiychuk-lohika commented Mar 7, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ymusiychuk-lohika Mar 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ymusiychuk-lohika Mar 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rozele Mar 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rozele commented Mar 8, 2017

Uh oh!

rozele commented Mar 9, 2017

Uh oh!

msftclas commented Mar 9, 2017

Uh oh!

ymusiychuk-lohika commented Mar 13, 2017

Uh oh!

codecov-io commented Apr 7, 2017

Codecov Report

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

ymusiychuk-lohika Mar 9, 2017 •

edited

Loading

ymusiychuk-lohika Mar 9, 2017 •

edited

Loading

rozele Mar 9, 2017 •

edited

Loading