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

Tooltip update #157

Merged
merged 38 commits into from
Oct 17, 2024
Merged

Tooltip update #157

merged 38 commits into from
Oct 17, 2024

Conversation

lunarias
Copy link
Collaborator

@lunarias lunarias commented Jul 2, 2024

Details found in issue #128

@lunarias lunarias self-assigned this Jul 2, 2024
@lunarias lunarias linked an issue Jul 2, 2024 that may be closed by this pull request
less/tooltip.less Outdated Show resolved Hide resolved
less/tooltip.less Outdated Show resolved Hide resolved
less/tooltip.less Outdated Show resolved Hide resolved
less/tooltip.less Outdated Show resolved Hide resolved
less/tooltip.less Outdated Show resolved Hide resolved
@lunarias lunarias changed the title [WIP] Tooltip update Tooltip update Jul 29, 2024
@lunarias lunarias requested a review from zoltan-dulac July 29, 2024 16:01
js/modules/es4/tooltip.js Show resolved Hide resolved
js/modules/es4/tooltip.js Outdated Show resolved Hide resolved
js/modules/es4/tooltip.js Outdated Show resolved Hide resolved
js/modules/es4/tooltip.js Outdated Show resolved Hide resolved
js/modules/es4/tooltip.js Outdated Show resolved Hide resolved
less/tooltip.less Show resolved Hide resolved
@lunarias lunarias requested a review from zoltan-dulac August 16, 2024 15:27
@zoltan-dulac
Copy link
Collaborator

Hi there. Just a few things that would make this a bit easier to use:

  1. When the user clicks on the tooltip button, you set the button's aria-describedby as tooltip. Instead of that, let's do the following:

    • On page load, set the aria-live="assertive" to <div id="tootip">.
    • If you do the first step, when the user clicks on a tooltip button, screen readers will announce the tooptip text that appears.
    • Please remove the aria-describedby logic. It shouldn't be there if the tooltip is activated on click (it should remain for the onfocus variation).
  2. You have two different types of tooltips -- one on focus and one on click. I would like to keep the onfocus variation, but I'd like to have it in a separate section, with a separate explanation about how it works.

  3. Let's change the copy with references of "I" to "We". The "I" implies I wrote it (and I did when I first posted this page, but there are a number of changes you have made that make this better).

  4. Put your name on the acknowledgements page. :-)

Copy link
Collaborator

@zoltan-dulac zoltan-dulac left a comment

Choose a reason for hiding this comment

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

Please read my last comment in the conversation tab -- there are a few things I'd like to do here before we sign off on this.

js/test/tooltip.test.js Show resolved Hide resolved
js/test/tooltip.test.js Outdated Show resolved Hide resolved
js/test/tooltip.test.js Outdated Show resolved Hide resolved
js/test/tooltip.test.js Outdated Show resolved Hide resolved
@lunarias lunarias requested a review from zoltan-dulac October 10, 2024 17:53
@zoltan-dulac zoltan-dulac merged commit d170968 into main Oct 17, 2024
2 checks passed
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.

Component Update: Tooltip
3 participants