-
Notifications
You must be signed in to change notification settings - Fork 105
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
fix: [IOBP-458,IOBP-459] Added accessibility label to the RNavScreenWithLargeHeader component #5467
fix: [IOBP-458,IOBP-459] Added accessibility label to the RNavScreenWithLargeHeader component #5467
Conversation
Affected stories |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #5467 +/- ##
=======================================
Coverage 47.82% 47.82%
=======================================
Files 1400 1400
Lines 30202 30203 +1
Branches 7379 7380 +1
=======================================
+ Hits 14443 14444 +1
Misses 15690 15690
Partials 69 69
Continue to review full report in Codecov by Sentry.
|
type Props = { | ||
children: React.ReactNode; | ||
fixedBottomSlot?: React.ReactNode; | ||
title: string; | ||
titleTestID?: string; | ||
title: LargeHeaderTitleProps; |
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.
title: LargeHeaderTitleProps; | |
titleProps: LargeHeaderTitleProps; |
since we are using this naming convention for the HeaderActions, I think this brings more consistency; what do you think?
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.
also, are you sure we want to force the a11y label instead of inferring it from the label if undef?
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.
About the renaming from title
to titleProps
I don't like it very much as a naming convention, indeed I'm following the naming convention also used on other components inside the design system library.
About the accessibilityLabel
as required, I agree with you and as a follow-up to today's meeting, I changed it as an optional property: 314de26
Short description
This PR adds the
accessibilityLabel
to the RNavScreenWithLargeHeader title and inside the new Transaction details screen has been added the role of the accessibility labelList of changes proposed in this pull request
accessibilityLabel
intotitle
RNavScreenWithLargeHeader propHow to test
Acknowledgment
@dmnplb @thisisjp