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.

Sign up for GitHub

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

Jump to bottom

chore: Update Text props aria-labelledby documentation #35327

Closed
wants to merge 1 commit into from
Closed

chore: Update Text props aria-labelledby documentation #35327

wants to merge 1 commit into from

Conversation

gabrieldonadel
Copy link
Collaborator

@gabrieldonadel gabrieldonadel commented Nov 14, 2022 •
edited
Loading

Summary

aria-labelledby Flow types were added to the Text props on f353119 but the text component does not support accessibilityLabelledBy, which is the prop we were supposed to map aria-labelledby to. As the Text component does not really support aria-labelledby this PR updates the TextProps documentation to explain that this prop is necessary for cross-platform purposes and that other platforms need it.

Changelog

[Android] [Changed] - Add notes to aria-labelledby from Text props

Test Plan

Ensure CI is green

@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 Nov 14, 2022
@gabrieldonadel gabrieldonadel mentioned this pull request Nov 14, 2022
Closed
@analysis-bot
Copy link

analysis-bot commented Nov 14, 2022 •
edited
Loading

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 7,101,325 +0
android hermes armeabi-v7a 6,471,026 +0
android hermes x86 7,519,166 +0
android hermes x86_64 7,377,786 +0
android jsc arm64-v8a 8,965,127 +0
android jsc armeabi-v7a 7,696,525 +0
android jsc x86 9,027,625 +0
android jsc x86_64 9,505,761 +0

Base commit: f453292
Branch: main

@necolas
Copy link
Contributor

necolas commented Nov 14, 2022

We'll keep it because it's for cross platform purpose, other platforms need it, and maybe Text for native can have it too one day

@necolas necolas closed this Nov 14, 2022
@analysis-bot
Copy link

analysis-bot commented Nov 14, 2022 •
edited
Loading

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

Base commit: a7ae988
Branch: main

@pull-bot
Copy link

PR build artifact for 632568c is ready.
To use, download tarball from "Artifacts" tab in this CircleCI job then run yarn add <path to tarball> in your React Native project.

@gabrieldonadel
Copy link
Collaborator Author

We'll keep it because it's for cross platform purpose, other platforms need it, and maybe Text for native can have it too one day

Got it, but wouldn't it be good if we have some comment explaining this? Just to prevent people like me from trying to remove it as there's no link between aria-labelledby and Text

@necolas
Copy link
Contributor

necolas commented Nov 14, 2022

Yeah good idea

@necolas necolas reopened this Nov 14, 2022
@gabrieldonadel gabrieldonadel changed the title chore: Remove aria-labelledby from Text props chore: Update Text props aria-labelledby documentation Nov 14, 2022
@gabrieldonadel gabrieldonadel force-pushed the chore/remove-aria-labelledby-from-text branch 2 times, most recently from 632568c to e9dd5c0 Compare November 14, 2022 20:06
@gabrieldonadel
Copy link
Collaborator Author

Yeah good idea

@necolas updated

@pull-bot
Copy link

PR build artifact for e30968e is ready.
To use, download tarball from "Artifacts" tab in this CircleCI job then run yarn add <path to tarball> in your React Native project.

@facebook-github-bot
Copy link
Contributor

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

@gabrieldonadel gabrieldonadel deleted the chore/remove-aria-labelledby-from-text branch November 15, 2022 01:25
OlimpiaZurek pushed a commit to OlimpiaZurek/react-native that referenced this pull request May 22, 2023
@gabrieldonadel @OlimpiaZurek
chore: Update Text props aria-labelledby documentation (facebook#35327)
74ccf0d 
Summary:
`aria-labelledby` Flow types were added to the Text props on facebook@f353119  but the text component does not support `accessibilityLabelledBy`, which is the prop we were supposed to map `aria-labelledby` to. As the Text component does not really support `aria-labelledby` this PR updates the `TextProps` documentation to explain that this prop is necessary for cross-platform purposes and that other platforms need it.

## Changelog

[Android] [Changed] - Add notes to `aria-labelledby` from Text props

Pull Request resolved: facebook#35327

Test Plan: Ensure CI is green

Reviewed By: necolas

Differential Revision: D41280877

Pulled By: lunaleaps

fbshipit-source-id: 81cf016bcf3117f15385ddd119c44d6f9fdd6b68
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@necolas necolas necolas approved these changes

Assignees
No one assigned
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@gabrieldonadel @analysis-bot @necolas @pull-bot @facebook-github-bot