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(mrf-approvals): improve copy for approvals #7902

Merged
merged 8 commits into from
Nov 18, 2024

Conversation

kevin9foong
Copy link
Contributor

@kevin9foong kevin9foong commented Nov 18, 2024

Problem

Some P1 improvements to the copy are requested to improve admin understandability for MRF Approvals.

Closes FRM-1910

Solution

Make the necessary changes according to the Figma.

Additional changes to aria-label for tooltip in FormLabel and Toggle components have been fixed for better screen reader accessibility. Previously, screen readers would just read tooltip label without reading the content of the tooltip.

Breaking Changes

  • No - this PR is backwards compatible

@kevin9foong kevin9foong requested a review from KenLSM November 18, 2024 02:33
@kevin9foong kevin9foong self-assigned this Nov 18, 2024
Copy link

linear bot commented Nov 18, 2024

@@ -92,7 +92,7 @@ export const FormLabel = ({
<Tooltip
placement={tooltipPlacement}
label={tooltipText}
aria-label="Label tooltip"
aria-label={`Tooltip content: ${tooltipText}`}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed to improve screen reader output to include tooltip text

<FormLabel.Label sx={{ ...styles.label, ...labelStyles }}>
{label}
</FormLabel.Label>
<Flex alignItems="center">
Copy link
Contributor Author

@kevin9foong kevin9foong Nov 18, 2024

Choose a reason for hiding this comment

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

no / minimal changes to previous usages of Tooltip expected

Rationale:

  • flex direction is row, with no changes to the main axis orientation
  • tooltipText is checked before adding another component. since prev usages do not pass in tooltipText, they should remain unchanged

@kevin9foong kevin9foong changed the title feat: approvals copy fixes feat: improve copy for approvals Nov 18, 2024
@kevin9foong kevin9foong changed the title feat: improve copy for approvals feat(mrf-approvals): improve copy for approvals Nov 18, 2024
Copy link
Contributor

@KenLSM KenLSM left a comment

Choose a reason for hiding this comment

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

LGTM

@kevin9foong kevin9foong merged commit 621bf76 into develop Nov 18, 2024
18 checks passed
@kevin9foong kevin9foong deleted the feat/approvals-fixes branch November 18, 2024 08:57
@kevin9foong kevin9foong mentioned this pull request Nov 19, 2024
14 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants