Implement WCAG-compliant localized common tooltips for disabled upload and file menu actions#3774
Conversation
Signed-off-by: yugalkaushik <yugalkaushik14@gmail.com>
| @@ -0,0 +1,79 @@ | |||
| import React, { ReactElement, useRef, useState } from 'react'; | |||
|
|
|||
There was a problem hiding this comment.
could we add some jsdocs for these types & tests for the new component?
Also tests would be great as the common folder has tests for most components now
I think it could be super basic like:
it('does not show the tooltip with the user is not hovering over the element', () => {
// render tooltip with some child content
// check screen that child content is present
// check screen that tooltip text content is not present
})
it('shows the tooltip if the user hovers over the element', () => {
// render tooltip with some child content
// trigger a userEvent to hover over the child element
// check screen that child content is present
// check screen that tooltip text content is not present
})
it('displays the tooltip in the direction passed', () => {})
it('defaults to north if the direction is not passed', () => {})There was a problem hiding this comment.
I added test coverage for the Tooltip component in Tooltip.test.tsx as well.
| const [open, setOpen] = useState(false); | ||
| const tooltipIdRef = useRef(`tooltip-${Math.random().toString(36).slice(2)}`); | ||
|
|
||
| const childProps: Record<string, unknown> = { |
There was a problem hiding this comment.
probably want to useMemo so this doesn't cause unnecessary re-renders
There was a problem hiding this comment.
syntax would be the below
const childProps = useMemo(() => ({...your object}),[...all the mentioned dependencies])There was a problem hiding this comment.
personally for readability, I would split this into different useMemos by property, especially bc you have the .filter().join() mixed with a bunch of event handlers
But this is a style opinion so not blocking
There was a problem hiding this comment.
Hi, I implemented useMemo for tooltipClasses and childProps to avoid unnecessary re-renders. I also simplified things quite a bit, removed all the event handlers and the .filter().join() chains since the tooltip is now handled purely via CSS using the ::after pseudo-element.
| */ | ||
| role?: MenubarItemRole; | ||
| selected?: boolean; | ||
| tooltipContent?: string; |
There was a problem hiding this comment.
I'd maybe link this to TooltipProps so that in case the Tooltip API changes, this component gets synced:
tooltipContent?: TooltipProps['content']
There was a problem hiding this comment.
Thanks, I’ve made this change. tooltipContent is now tied to TooltipProps['content'] instead of being a plain string, so it’ll stay in sync if the Tooltip API changes.
There was a problem hiding this comment.
I'm not 100% familiar with how translations work
Does the translator default to check en-US if say fr-FR['SaveTooltipUnauthenticated'] doesn't work @raclim?
If not you might have to pop the temporary english into each of the language files as placeholders -- I thiiink this is the current pattern I've seen but not 10000% sure
There was a problem hiding this comment.
@clairep94 Yes, the repo already falls back to English if a translation is missing. So if a key isn’t present in something like fr-FR, it’ll just use the en-US version automatically. Because of that, there’s no need to add English placeholders to every language file. You can just add new keys to en-US/translations.json and they’ll work everywhere until proper translations are added.
There was a problem hiding this comment.
Yes, the repo does default to en-US! I think it would probably be good to have the placeholder text added in to the other files to help maintain consistency, though I know we already have a few files that aren't in-sync.
We might want to open a new issue once this is in!
|
@clairep94 Hi, this code needs some refinement, which I will address shortly. At the moment, I’m unable to spend enough time working on it. |
Signed-off-by: yugalkaushik <yugalkaushik14@gmail.com>
|
@raclim this PR should be good to go and ready for merge after your review. Thanks @clairep94 for the suggestions. |
|
Thanks so much @clairep94 for the thorough review and @yugalkaushik for building this—it looks really solid! The next deploy won't be until probably next week earliest, so feel free to add anything in if anything comes up! |
Fixes #3773, #3511, #3079


We can close the respective PRs #3514 #3497
Changes:
I have verified that this pull request:
npm run lint)npm run test)npm run typecheck)developbranch.Fixes #123