-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Implement WCAG-compliant localized common tooltips for disabled upload and file menu actions #3774
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
base: develop
Are you sure you want to change the base?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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', () => {})| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
probably want to useMemo so this doesn't cause unnecessary re-renders
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
syntax would be the below
const childProps = useMemo(() => ({...your object}),[...all the mentioned dependencies])There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
| */ | ||
| role?: MenubarItemRole; | ||
| selected?: boolean; | ||
| tooltipContent?: string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
|
@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. |
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