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

Dashboard: add tooltip to not analyzed #21870

Open
wants to merge 8 commits into
base: feature/dash-phase-1
Choose a base branch
from

Conversation

igorschoester
Copy link
Member

@igorschoester igorschoester commented Nov 27, 2024

Context

Summary

This PR can be summarized in the following changelog entry:

  • Adds a tooltip with information to the not analyzed scores.
  • [@yoast/ui-library 0.1.0 enhancement] Adds a TooltipContainer component that handles a11y functionality around the Tooltip element.
  • [@yoast/ui-library 0.1.0 enhancement] Adds a useKeydown hook to handle the keydown event listener subscription.

Relevant technical choices:

  • Moved the list item to its own component to be able to track the visible per item.
  • Confirmed by UX: hovering over the bullet + label + badge will show the tooltip, and popping out from there too (the centering of the balloon arrow thing)
  • aria-describedby only when there is a tooltip (passing undefined will stop the attribute from being added)
  • The tooltip is not fully a11y compatible due to no element to focus on and our UI library needing improvements
  • Created a UI library implementation

Improve a11y of the tooltip

  • add aria-describedby that needs an ID
  • use button as a focusable element, this is a hacky solution
  • add aria-disabled to the button to indicate that it does nothing
  • show the tooltip on focus and hover
  • hide the tooltip on Escape press
  • move the tooltip out of the button so it does not influence the content (an alternative, less ideal would be to add aria-hidden)
  • add a wrapper to still get the tooltip positioning with relative
  • use focus-visible so the keyboard user still sees the focus

Test instructions

Test instructions for the acceptance test before the PR gets merged

This PR can be acceptance tested by following these steps:

  • Go to the dashboard
  • Hover over the Not analyzed scores (both SEO & readability)
  • Verify the tooltip shows
  • Verify the copy is per the design: We haven’t analyzed this content yet. Please open it in your editor, ensure a focus keyphrase is entered, and save it so we can start the analysis.
  • Verify that pressing escape while hovering closes the tooltip
  • Navigate to the Not analyzed by keyboard
  • Verify the tooltip shows
  • Verify you see the focus border
  • Verify that pressing escape while hovering closes the tooltip
  • Shrink the width of the screen
  • Verify the tooltip goes to a width similar to the score + label + badge and so remains visible/on the screen
  • If you have access to say Safari, you could open the dashboard there and activate VoiceOver and verify that the tooltip text is spoken out loud when Not analyzed receives focus.
  • Select the button (class: yst-tooltip-trigger) around the Not analyzed in the inspector, and open the Accessibility tab
  • Verify the Name is Not analyzed followed by the amount, e.g. 3
  • Verify the Description is the contents of the tooltip

[DEV only] Storybook

  • Start the storybook yarn workspace @yoast/ui-library storybook
  • Go to Components > Tooltip container
  • Does the text make sense?
  • Does the story portray the components?
  • Might do a follow-up instead if you have too many comments 😁
  • For regression: go to Elements > Tooltip
  • Verify the tooltip still works the same there

Regression

  • Edit a post
  • Ensure you have a keyphrase
  • Click on Get related keyphrases
  • Be sure to connect to Semrush so you get the modal with results
  • Hover over the intent and difficulty percentages
  • Verify you get the tooltips still

Relevant test scenarios

  • Changes should be tested with the browser console open
  • Changes should be tested on different posts/pages/taxonomies/custom post types/custom taxonomies
  • Changes should be tested on different editors (Default Block/Gutenberg/Classic/Elementor/other)
  • Changes should be tested on different browsers
  • Changes should be tested on multisite

Test instructions for QA when the code is in the RC

  • QA should use the same steps as above.

QA can test this PR by following these steps:

Impact check

This PR affects the following parts of the plugin, which may require extra testing:

  • The tooltip in the UI library and the scores in the dashboard

UI changes

  • This PR changes the UI in the plugin. I have added the 'UI change' label to this PR.

Other environments

  • This PR also affects Shopify. I have added a changelog entry starting with [shopify-seo], added test instructions for Shopify and attached the Shopify label to this PR.

Documentation

  • I have written documentation for this change. For example, comments in the Relevant technical choices, comments in the code, documentation on Confluence / shared Google Drive / Yoast developer portal, or other.

Quality assurance

  • I have tested this code to the best of my abilities.
  • During testing, I had activated all plugins that Yoast SEO provides integrations for.
  • I have added unit tests to verify the code works as intended.
  • If any part of the code is behind a feature flag, my test instructions also cover cases where the feature flag is switched off.
  • I have written this PR in accordance with my team's definition of done.
  • I have checked that the base branch is correctly set.

Innovation

  • No innovation project is applicable for this PR.
  • This PR falls under an innovation project. I have attached the innovation label.
  • I have added my hours to the WBSO document.

Fixes https://github.com/Yoast/reserved-tasks/issues/337

See: https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Roles/tooltip_role
* add aria-describedby that needs an ID
* use button as a focusable element, this is a hacky solution
* add aria-disabled to the button to indicate that it does nothing
* show the tooltip on focus and hover
* hide the tooltip on Escape press
* move the tooltip out of the button so it does not influence the content (an alternative, less ideal would be to add aria-hidden)
* add a wrapper to still get the tooltip positioning with relative
* use focus-visible so the keyboard user still sees the focus
Missing: adjustment of the tooltip display styling. As that is inside the UI library, more commits incoming to try to move this solution there.
@igorschoester igorschoester added the changelog: non-user-facing Needs to be included in the 'Non-userfacing' category in the changelog label Nov 27, 2024
@igorschoester igorschoester added this to the feature/dash-phase-1 milestone Nov 27, 2024
@igorschoester igorschoester requested a review from a team as a code owner November 27, 2024 15:40
Components to use the Tooltip element in a a11y friendly manner
@igorschoester igorschoester force-pushed the 337-add-tool-tip-for-not-analyzed-label branch from e240361 to 68a9c95 Compare November 27, 2024 15:53
@coveralls
Copy link

coveralls commented Nov 27, 2024

Pull Request Test Coverage Report for Build 01c57b68a1d3880115f7af3a0793323b75d43062

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 0 of 11 (0.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.007%) to 54.094%

Changes Missing Coverage Covered Lines Changed/Added Lines %
packages/js/src/dashboard/scores/components/score-content.js 0 1 0.0%
packages/js/src/dashboard/scores/components/score-list.js 0 10 0.0%
Totals Coverage Status
Change from base Build 0961ff39da8949542fbd4b23ffb049723c44e9e2: -0.007%
Covered Lines: 29774
Relevant Lines: 55400

💛 - Coveralls

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog: non-user-facing Needs to be included in the 'Non-userfacing' category in the changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants