Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Text] Fix text not wrapping #2742

Draft
wants to merge 14 commits into
base: main
Choose a base branch
from
Draft

Conversation

ayush547
Copy link
Contributor

@ayush547 ayush547 commented Apr 3, 2023

Platforms Impacted

  • iOS
  • macOS
  • win32 (Office)
  • windows
  • android

Description of changes

  • Added default accessibilityRole: 'text' to text component
  • RN Text contains a bug where it does not wrap properly and can lead to long text going beyond the screen (see attached images for example).
    This PR fixes that.
    (FlexShrink property was added to fix this, it is till overridable through props)

Verification

Android developer option - Layout Bounds has been enabled for better visualization.

Before After
Screenshot_2023-03-31-14-33-34-63_3238b23fbbdef38ccf4a465a34706bab Screenshot_2023-03-31-14-30-36-88_3238b23fbbdef38ccf4a465a34706bab
Screenshot_2023-03-31-14-33-58-32_3238b23fbbdef38ccf4a465a34706bab Screenshot_2023-03-31-14-30-51-73_3238b23fbbdef38ccf4a465a34706bab
"uv" is hidden beyond "st" off the screen text moves to next line correctly

Pull request checklist

This PR has considered (when applicable):

  • Automated Tests
  • Documentation and examples
  • Keyboard Accessibility
  • Voiceover
  • Internationalization and Right-to-left Layouts

@ayush547 ayush547 requested a review from a team as a code owner April 3, 2023 08:47
@ayush547 ayush547 marked this pull request as draft April 3, 2023 11:46
@@ -21,7 +21,7 @@ function onChangeUncontrolled(_e: InteractionEvent, isChecked: boolean) {
const BasicCheckbox: React.FunctionComponent = () => {
return (
<View>
<Checkbox label="Unchecked checkbox (undefined)" onChange={onChangeUncontrolled} />
<Checkbox label="Unchecked checkbox (undefined) aidasdijk ahsdihsaidhas jasdhasid" onChange={onChangeUncontrolled} />
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should leave the previous text only.
As we have already done our testing, this is not required for tester.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or may be add a bigger valid(meaningful) string?

@@ -96,6 +96,7 @@ export const Text = compressible<TextProps, TextTokens>((props: TextProps, useTo
// now build the text style from tokens that can be shared between different Text instances
const [tokenStyle] = cache(
() => ({
flexShrink: 1, // Fixes bug in RN where text does not wrap correctly. RN bug #1438
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am wondering why Checkbox and Switch snapshot didn't change as they have used the Text same as Radio.

If we have redundant flexShrink:1 on Checkbox and Switch , please remove that from there and let's have it on Text only.

@@ -138,7 +139,8 @@ export const Text = compressible<TextProps, TextTokens>((props: TextProps, useTo

// return a continuation function that allows this text to be compressed
return (extra: TextProps, children: React.ReactNode) => {
const mergedProps = {
const mergedProps: TextProps = {
accessibilityRole: 'text',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is good addition. 👍

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missed this, same fix was made for Switch for accessibility issue here: #2644

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants