Exposing the maxContentSizeMultiplier prop for RN.Text and RN.TextInput. by rumit91 · Pull Request #78 · microsoft/reactxp

rumit91 · 2017-05-01T22:23:09Z

Whenever a user changes the system font size to its maximum allowable setting most React Native apps that allow font scaling become unusable. Ideally React Native apps would mimic the behavior of the iMessage (iOS) app where the font size used for non-body text (header, navigational elements, etc) is capped while the body text (text in the message bubbles) is allowed to grow. In this PR I expose the maxContentSizeMultiplier prop on <Text>, <TextInput>, and <Link>. Developers can overwrite the default maxContentSizeMultiplier by calling the static method UserInterface. setMaxContentSizeMultiplier() or they can just choose to set the maxContentSizeMultiplier on the desired elements.

…with minimumFontScale.

… Native.

msftclas · 2017-05-01T22:23:13Z

@rumit91,
Thanks for your contribution as a Microsoft full-time employee or intern. You do not need to sign a CLA.
Thanks,
Microsoft Pull Request Bot

erictraut · 2017-05-01T22:37:48Z

package.json

    "react-addons-test-utils": "^15.4.1",
    "react-dom": "^15.4.1",
-    "react-native": "^0.42.0",
+    "react-native": "git+https://skype.visualstudio.com/DefaultCollection/TPS/_git/react-native#timale/fontScalingLimit",


You can't change the public reactxp to point to a private, internal react-native version. Just leave the version as-is. Internally, the Skype repo can refer to the private react-native branch.

Yep will remove for sure. Was just using this for testing.

erictraut · 2017-05-01T22:38:07Z

src/android/Text.tsx

                importantForAccessibility={ importantForAccessibility }
                numberOfLines={ this.props.numberOfLines === 0 ? null : this.props.numberOfLines }
                allowFontScaling={ this.props.allowFontScaling }
+                maxFontSizeMultiplier={ this.props.maxFontSizeMultiplier }


Why wouldn't we also expose a minFontSizeMultiplier?

I didn't add a min, because the user changing the system font to the minimum didn't seem to cause any issues in RN apps, but do you think it would still be valuable for me to add?

erictraut · 2017-05-01T22:39:18Z

src/android/Text.tsx

        AccessibilityUtil.setAccessibilityFocus(this);
    }
+
+    static setDefaultMaxFontSizeMultiplier(maxFontSizeMultiplier: number): void {


This shouldn't be exposed as a static method on Text. It should be a method on the UserInterface namespace.

Moved to UserInterface.

erictraut · 2017-05-01T22:40:34Z

src/android/Text.tsx

                importantForAccessibility={ importantForAccessibility }
                numberOfLines={ this.props.numberOfLines === 0 ? null : this.props.numberOfLines }
                allowFontScaling={ this.props.allowFontScaling }
+                maxFontSizeMultiplier={ this.props.maxFontSizeMultiplier }


For consistency with UserInterface (and react native) terminology, this should be called maxContentSizeMultiplier, not minFontSizeMultiplier.

Changed the var name.

erictraut · 2017-05-01T22:41:19Z

src/android/Text.tsx

                importantForAccessibility={ importantForAccessibility }
                numberOfLines={ this.props.numberOfLines === 0 ? null : this.props.numberOfLines }
                allowFontScaling={ this.props.allowFontScaling }
+                maxFontSizeMultiplier={ this.props.maxFontSizeMultiplier }


Do we really need to expose an override of the default max on a per-Text basis? This seems like it might be overkill.

I think we need this to be able to override the default in specific instances. For example, we may want the maxContentSizeMultiplier to be much higher for text inside message bubbles vs the text in the navigational elements.

erictraut · 2017-05-01T22:43:36Z

src/native-common/TextInput.tsx

        this._onChangeText(value);
    }
+
+    static setDefaultMaxFontSizeMultiplier(maxFontSizeMultiplier: number): void {


I don't think we need a different default for Text and TextInput. As mentioned above, this should be collapsed to a single API - UserInterface.setDefaultMaxContentSizeMultiplier.

Agreed. Moved to UserInterface.

erictraut · 2017-05-01T22:44:33Z

src/android/Text.tsx

                importantForAccessibility={ importantForAccessibility }
                numberOfLines={ this.props.numberOfLines === 0 ? null : this.props.numberOfLines }
                allowFontScaling={ this.props.allowFontScaling }
+                maxFontSizeMultiplier={ this.props.maxFontSizeMultiplier }


The documentation for Text, TextInput and UserInterface need to be updated as well.

Thanks for reminding me. I added the docs.

…Added `maxContentSizeMultiplier` to Link.

…tly not available in most version of RN.

erictraut · 2017-05-08T14:26:11Z

docs/docs/apis/userinterface.md

+
+// Triggered when the max content size multiplier changes while
+// the app is running.
+maxContentSizeMultiplierChangedEvent: SubscribableEvent<


We don't need this event. The max content size multiplier is under the control of the app, so it can track this change itself. The reason I added an event for contentSizeMultiplierChanged is that it's under control of the OS (and ultimately the user), so the app needs to be informed when it changes.

Removed from the code and the docs.

erictraut · 2017-05-08T14:28:27Z

docs/docs/components/text.md

+// Should the scale multiplier be capped when allowFontScaling is set to true?
+// The default of 0 indicates that the compoent should obey the global setting 
+// in UserInterface which by default is uncapped.
+// Note: Most versions of React Native don’t support this interface. 


erictraut · 2017-05-08T14:28:47Z

docs/docs/components/textinput.md

+// Should the scale multiplier be capped when allowFontScaling is set to true?
+// The default of 0 indicates that the compoent should obey the global setting 
+// in UserInterface which by default is uncapped.
+// Note: Most versions of React Native don’t support this interface. 


erictraut · 2017-05-08T14:29:20Z

docs/docs/components/link.md

+// Should the scale multiplier be capped when allowFontScaling is set to true?
+// The default of 0 indicates that the compoent should obey the global setting 
+// in UserInterface which by default is uncapped.
+// Note: Most versions of React Native don’t support this interface. 


Rather than saying "Most versions of React Native", please change to "Older versions...".

erictraut · 2017-05-08T14:30:11Z

src/android/Text.tsx

            </RN.Text>
        );
    }
-


Did you intend to delete this, or was it a merge error? Amay recently added this code.

I deleted it because it caused merge conflicts. I believe Amay removed it from master recently with this commit: cff4b51

erictraut

When you're ready to merge this, let DDR or Brent know, and they can merge and republish.

…ut. (microsoft#78) * Exposed the maximumFontScale prop and updating docs

rumit91 added 9 commits April 26, 2017 18:49

Allow dist folder on this branch.

015d5b2

Updated to point to my fork of react native.

02b3246

Checking in the dist folder.

fd14ceb

Exposed the maximumFontScale prop from React Native.

45671c3

Dist folder changed.

7ade2fd

Renamed maximumFontScale to maxFontSizeMultiplier to avoid confusion …

1b2a120

…with minimumFontScale.

Added the maxFontSizeMultiplier to Android.

b76cfe7

Piped through the setDefaultMaxFontSizeMultiplier() method from React…

d0388e3

… Native.

Added the ability to set the maxFontSizeMultiplier on <TextInput>.

b242b6b

msftclas added the cla-not-required label May 1, 2017

rumit91 added 2 commits May 1, 2017 15:27

Adding the dist folder back to the gitignore.

b6390f4

Deleting the dist folder.

60f855b

erictraut suggested changes May 1, 2017

View reviewed changes

rumit91 added 7 commits May 1, 2017 16:10

Fixing a merge conflict.

abebd22

Renamed maxFontSizeMultiplier to maxContentSizeMultiplier.

fcdfee7

Moved the setting the default maxContentSizeMultiplier to UserInterface.

5029d1a

Updated the docs.

ca1f0d0

Removed setDefaultContentSizeMultiplier() from Text and TextInput. …

d19c1ac

…Added `maxContentSizeMultiplier` to Link.

Reverting back to the public version of react-native.

8894efc

Updated the docs to reflect that maxContentSizeMultiplier is curren…

dff2b5e

…tly not available in most version of RN.

rumit91 changed the title ~~Exposing the maxFontSizeMultiplier prop for RN.Text and RN.TextInput.~~ Exposing the maxContentSizeMultiplier prop for RN.Text and RN.TextInput. May 6, 2017

erictraut suggested changes May 8, 2017

View reviewed changes

rumit91 added 2 commits May 8, 2017 15:18

Addressed PR comments.

9cf6202

Fixed a minor bug with the previous commit.

ad693ac

erictraut approved these changes May 9, 2017

View reviewed changes

rumit91 added 3 commits May 9, 2017 12:30

Updated the docs based on PR suggestions.

4695527

setMaxContentSizeMultiplier() now returns void instead of a promise.

314762a

Updated the docs based on corresponding changed to RN.

972ea8c

berickson1 merged commit 7991b17 into microsoft:master May 11, 2017

berickson1 pushed a commit to berickson1/reactxp that referenced this pull request Oct 22, 2018

Exposing the maxContentSizeMultiplier prop for RN.Text and RN.TextInp…

3bab16b

…ut. (microsoft#78) * Exposed the maximumFontScale prop and updating docs

Conversation

rumit91 commented May 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

msftclas commented May 1, 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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

erictraut left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

rumit91 commented May 1, 2017 •

edited

Loading