-
Notifications
You must be signed in to change notification settings - Fork 2
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
Update CONSTS to direct developers to use role instead of accessibilityRole (#31500) #74
Update CONSTS to direct developers to use role instead of accessibilityRole (#31500) #74
Conversation
…BLITY_ROLE.TEXT` to point to `ROLE.PRESENTATION`
…g button` value by reverting them to `accessibilityRole`
…eprecatd CONST.ACCESSIBLITY_ROLE
…eprecatd CONST.ACCESSIBLITY_ROLE
…eprecatd CONST.ACCESSIBLITY_ROLE
…deprecatd CONST.ACCESSIBLITY_ROLE
…BLITY_ROLE with CONST.ROLE
@trevor-coleman looks like the linter isn't happy! And some conflicts |
Also we should probably point it at a different branch than |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Love the detail in documentation
Signed-off-by: Trevor Coleman <[email protected]>
Linter passing -- going to make that new branch now. |
@cdanwards -- lints are passing, and this is now against a feature branch. I think she's ready to go. |
Yeah when I do big find-replaces like this I like to document the edge cases to (hopefully) make it easier to review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This fixes the issue with ROLE and ACCESSIBLITY_ROLE identified earlier and discussed on Slack.
It makes the following changes:
Changes to
CONST.ts
CONST.ACCESSIBILITY_ROLE.TEXT
to the value'text'
CONST.ACCESSIBLITY_ROLE
and all propertiesCONST.ROLE
object with JSDoc commentsFixes to Components
Simple find-replace
The majority of components were fixed using a simple find and replaced that made the following change
Replace:
role={CONST.ACCESSIBILITY_ROLE
withrole={CONST.ROLE
For example:
role={CONST.ACCESSIBILITY_ROLE.BUTTON}
would becomerole={CONST.ROLE.BUTTON}
Specific Cases
TEXT
to meanPRESENTATION
role={CONST.ACCESSIBILITY_ROLE.TEXT}
role={CONST.ROLE.PRESENTATION}
IMAGEBUTTON
IMAGEBUTTON
only available inaccessibilityRole
. Corrected property toaccessibilityRole
and pointed to the right constrole={CONST.ACCESSIBILITY_ROLE.IMAGEBUTTON}
accessibilityRole={CONST.ACCESSIBILITY_ROLE.IMAGEBUTTON}
ADJUSTABLE
adjustable
is not available inrole
but is inaccessibilityRole
. Reverted the value and updated prop to reflect correct usagerole={CONST.ACCESSIBILITY_ROLE.ADJUSTABLE}
role={CONST.ROLE.SLIDER}
ACCESSIBILITY_ROLE.IMAGE
role
valuerole={CONST.ACCESSIBILITY_ROLE.IMAGE}
role={CONST.ROLE.IMG}
Note: In the IMAGEBUTTON Case, I thought it was best to preserve the
imagebutton
role information, and minimize the number of changes, rather than try to decide on a case-by-case basis what an appropriate replacement would be.Details
Fixed Issues
$
PROPOSAL:
Tests
Offline tests
QA Steps
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop