Skip to content

feat: Add role prop to Text component - #34976

Closed
gabrieldonadel wants to merge 2 commits into
react:mainfrom
gabrieldonadel:feat/add-role-to-text
Closed

feat: Add role prop to Text component#34976
gabrieldonadel wants to merge 2 commits into
react:mainfrom
gabrieldonadel:feat/add-role-to-text

Conversation

@gabrieldonadel

Copy link
Copy Markdown
Collaborator

Summary

As pointed out by @necolas on #34424 (comment) we forgot we add the role prop mapping to the Text component. This PR adds a new role prop to Text, mapping the web role values to the already existing accessibilityRole prop and moves the roleToAccessibilityRoleMapping to a common file that can be imported by both the Text and View components as requested on #34424. This PR also updates the RNTester AcessebilityExample to include a test using this new prop.

Changelog

[General] [Added] - Add role prop to Text component

Test Plan

  1. Open the RNTester app and navigate to the Accessibility Example page
  2. Test the role prop through the Text with role = heading section

@facebook-github-botfacebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Oct 14, 2022

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

These were missing on TS

@react-native-botreact-native-bot added the Type: Enhancement A new feature or enhancement of an existing feature. label Oct 14, 2022
@analysis-bot

analysis-bot commented Oct 14, 2022

Copy link
Copy Markdown
PlatformEngineArchSize (bytes)Diff
androidhermesarm64-v8a7,777,909+486
androidhermesarmeabi-v7a7,178,967+537
androidhermesx868,090,829+399
androidhermesx86_648,062,605+485
androidjscarm64-v8a9,636,993+112
androidjscarmeabi-v7a8,401,283+115
androidjscx869,586,189+131
androidjscx86_6410,179,350+124

Base commit: 5d8a712
Branch: main

@analysis-bot

analysis-bot commented Oct 14, 2022

Copy link
Copy Markdown
PlatformEngineArchSize (bytes)Diff
ios-universaln/a--

Base commit: 5d8a712
Branch: main

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This object roleToAccessibilityRoleMapping needs to in JS VM heap all time after loading this JS. Consider making this is getter function so this object can be deallocated after the call, or better use a switch statement like this.
https://github.com/facebook/react-native/blob/84225573c17b17844b3b3e2a438b3178f7d35b05/packages/normalize-color/index.js#L303

cc @javache

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

Sure, I've just refactor it to be a switch statement

facebook-github-bot referenced this pull request Oct 14, 2022
Summary:
I've been noticing this for a while now, every time that someone changes a file inside rn-tester Danger comments a bunch of warnings regarding inline styles, e.g. #34567 (comment), even though that is disabled inside .eslintrc https://github.com/facebook/react-native/blob/8edf4e9e3adcc85743e3c86a25ab3748b276a3da/packages/rn-tester/.eslintrc#L3
After some investigation, I realized that the problem was that the Eslint Node.js API used by [seadub/danger-plugin-eslint](https://www.npmjs.com/package/seadub/danger-plugin-eslint) was not able to locate any `.eslintrc` files due to the location of the directory where danger is invoked. By using `process.chdir` we can ensure that eslint will run from the root folder and that it will be able to find all .eslintrc files
## Changelog
[Internal] [Fixed] - fix eslint config when running Danger
Pull Request resolved: #34980
Test Plan:
run `yarn danger pr https://github.com/facebook/react-native/pull/34976`
After
![image](https://user-images.githubusercontent.com/11707729/195751496-5225a32f-f4b3-4ce2-833f-ae5723f647c0.png)
Before
![image](https://user-images.githubusercontent.com/11707729/195751673-34ba87fc-ce50-4020-9688-a486e3021c4f.png)
Reviewed By: cortinico
Differential Revision: D40384300
Pulled By: yungsters
fbshipit-source-id: e68eeafc42567dc9d7297dde3709a989cc70f4e2
mohitcharkha referenced this pull request in mohitcharkha/react-native Oct 17, 2022
Summary:
I've been noticing this for a while now, every time that someone changes a file inside rn-tester Danger comments a bunch of warnings regarding inline styles, e.g. react#34567 (comment), even though that is disabled inside .eslintrc https://github.com/facebook/react-native/blob/8edf4e9e3adcc85743e3c86a25ab3748b276a3da/packages/rn-tester/.eslintrc#L3
After some investigation, I realized that the problem was that the Eslint Node.js API used by [seadub/danger-plugin-eslint](https://www.npmjs.com/package/seadub/danger-plugin-eslint) was not able to locate any `.eslintrc` files due to the location of the directory where danger is invoked. By using `process.chdir` we can ensure that eslint will run from the root folder and that it will be able to find all .eslintrc files
## Changelog
[Internal] [Fixed] - fix eslint config when running Danger
Pull Request resolved: react#34980
Test Plan:
run `yarn danger pr https://github.com/facebook/react-native/pull/34976`
After
![image](https://user-images.githubusercontent.com/11707729/195751496-5225a32f-f4b3-4ce2-833f-ae5723f647c0.png)
Before
![image](https://user-images.githubusercontent.com/11707729/195751673-34ba87fc-ce50-4020-9688-a486e3021c4f.png)
Reviewed By: cortinico
Differential Revision: D40384300
Pulled By: yungsters
fbshipit-source-id: e68eeafc42567dc9d7297dde3709a989cc70f4e2
@gabrieldonadel

Copy link
Copy Markdown
CollaboratorAuthor

@jacdebug would you mind rereviwing this PR?

@jacdebugjacdebug left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for the fixups, LGTM and sorry for delay!

@facebook-github-bot

Copy link
Copy Markdown
Contributor

@jacdebug has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@gabrieldonadel
gabrieldonadel deleted the feat/add-role-to-text branch October 24, 2022 15:44
@react-native-bot

Copy link
Copy Markdown
Collaborator

This pull request was successfully merged by @gabrieldonadel in 20718e6.

When will my fix make it into a release? | Upcoming Releases

@react-native-botreact-native-bot added the Merged This PR has been merged. label Oct 24, 2022
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA SignedThis label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.MergedThis PR has been merged.Type: EnhancementA new feature or enhancement of an existing feature.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

@gabrieldonadel@analysis-bot@facebook-github-bot@react-native-bot@necolas@jacdebug