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

feat: Add role prop to Text component #34976

Closed

Conversation

gabrieldonadel
Copy link
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-bot facebook-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
@@ -286,3 +291,69 @@ export interface AccessibilityPropsIOS {
*/
accessibilityIgnoresInvertColors?: boolean | undefined;
}

export type Role =
Copy link
Collaborator Author

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-bot react-native-bot added the Type: Enhancement A new feature or enhancement of an existing feature. label Oct 14, 2022
@analysis-bot
Copy link

analysis-bot commented Oct 14, 2022

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 7,777,909 +486
android hermes armeabi-v7a 7,178,967 +537
android hermes x86 8,090,829 +399
android hermes x86_64 8,062,605 +485
android jsc arm64-v8a 9,636,993 +112
android jsc armeabi-v7a 8,401,283 +115
android jsc x86 9,586,189 +131
android jsc x86_64 10,179,350 +124

Base commit: 5d8a712
Branch: main

@analysis-bot
Copy link

analysis-bot commented Oct 14, 2022

Platform Engine Arch Size (bytes) Diff
ios - universal n/a --

Base commit: 5d8a712
Branch: main

'use strict';

// Map role values to AccessibilityRole values
export const roleToAccessibilityRoleMapping = {
Copy link
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.

function normalizeKeyword(name) {

cc @javache

Copy link
Collaborator Author

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. facebook#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: facebook#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
Collaborator Author

@jacdebug would you mind rereviwing this PR?

Copy link
Contributor

@jacdebug jacdebug left a comment

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
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
Collaborator

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

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

@react-native-bot react-native-bot added the Merged This PR has been merged. label Oct 24, 2022
OlimpiaZurek referenced this pull request in OlimpiaZurek/react-native May 22, 2023
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. facebook#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: facebook#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
OlimpiaZurek pushed a commit to OlimpiaZurek/react-native that referenced this pull request May 22, 2023
Summary:
As pointed out by necolas on facebook#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 facebook#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

Pull Request resolved: facebook#34976

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

Reviewed By: yungsters

Differential Revision: D40596039

Pulled By: jacdebug

fbshipit-source-id: f72f02e8bd32169423ea517ad18b598b52257b17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged This PR has been merged. Type: Enhancement A new feature or enhancement of an existing feature.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants