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

fix: [IOBP-458,IOBP-459] Added accessibility label to the RNavScreenWithLargeHeader component #5467

Merged
merged 12 commits into from
Feb 6, 2024

Conversation

Hantex9
Copy link
Contributor

@Hantex9 Hantex9 commented Jan 30, 2024

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 label

List of changes proposed in this pull request

  • Added accessibilityLabel into title RNavScreenWithLargeHeader prop
  • Adapted the already used RNavScreenWithLargeHeader with the new title object prop

How to test

  • Enable the DS flag
  • With the voiceover enabled, navigate to the new Transaction Screen details
  • You should be able to hear the "heading" role by the voiceover tapping on the header title

Acknowledgment

@dmnplb @thisisjp

@Hantex9 Hantex9 added the IO-Bonus e pagamenti IO - Bonus e pagamenti label Jan 30, 2024
@Hantex9 Hantex9 requested review from dmnplb and a team as code owners January 30, 2024 17:35
@pagopa-github-bot
Copy link
Collaborator

pagopa-github-bot commented Jan 30, 2024

Warnings
⚠️

Multiple stories with different types are associated with this Pull request.
Only one tag will be added, following the order: feature > bug > chore

Affected stories

  • 🐞 IOBP-458: [A11y] List item header - Ruolo non letto
  • 🐞 IOBP-459: [A11y] Titolo - ruolo mancante

Generated by 🚫 dangerJS against 2b7cfea

Copy link

codecov bot commented Jan 30, 2024

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (63fd1c2) 47.82% compared to head (2b7cfea) 47.82%.

Additional details and impacted files

Impacted file tree graph

@@           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           
Files Coverage Δ
ts/components/ui/RNavScreenWithLargeHeader.tsx 80.95% <100.00%> (+0.95%) ⬆️
...dpay/details/screens/IdPayOperationsListScreen.tsx 8.82% <ø> (ø)
...saction/screens/WalletTransactionDetailsScreen.tsx 16.66% <ø> (ø)
...tion/screens/WalletTransactionOperationDetails.tsx 8.69% <ø> (ø)
.../onboarding/OnboardingServicesPreferenceScreen.tsx 7.69% <ø> (ø)
...s/screens/onboarding/OnboardingShareDataScreen.tsx 14.28% <ø> (ø)
ts/screens/profile/CalendarsPreferencesScreen.tsx 16.66% <ø> (ø)
ts/screens/profile/LanguagesPreferencesScreen.tsx 8.88% <ø> (ø)
...screens/profile/NotificationsPreferencesScreen.tsx 54.54% <ø> (ø)
ts/screens/profile/PreferencesScreen.tsx 5.35% <ø> (ø)
... and 8 more

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 63fd1c2...2b7cfea. Read the comment docs.

type Props = {
children: React.ReactNode;
fixedBottomSlot?: React.ReactNode;
title: string;
titleTestID?: string;
title: LargeHeaderTitleProps;
Copy link
Contributor

@forrest57 forrest57 Jan 31, 2024

Choose a reason for hiding this comment

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

Suggested change
title: LargeHeaderTitleProps;
titleProps: LargeHeaderTitleProps;

since we are using this naming convention for the HeaderActions, I think this brings more consistency; what do you think?

Copy link
Contributor

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?

Copy link
Contributor Author

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

@Hantex9 Hantex9 merged commit a57d5e2 into master Feb 6, 2024
8 checks passed
@Hantex9 Hantex9 deleted the IOBP-458-459-accessibility-title-scroll-header branch February 6, 2024 08:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
IO-Bonus e pagamenti IO - Bonus e pagamenti
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants