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.

Sign up for GitHub

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

minor fix -- console logs #9560

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
+43 −13
Open

minor fix -- console logs #9560

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c8e0157
fix
ehconitin Jan 11, 2025
5a1efd7
grep
ehconitin Jan 11, 2025
86ab356
Merge branch 'main' into console-logs-fix
ehconitin Jan 24, 2025
0878d42
charles review
ehconitin Jan 24, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions packages/twenty-front/src/modules/favorites/components/CurrentWorkspaceMemberFavorites.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,7 @@ export const CurrentWorkspaceMemberFavorites = ({
onRename={() => setIsFavoriteFolderRenaming(true)}
onDelete={handleFavoriteFolderDelete}
closeDropdown={closeFavoriteFolderEditDropdown}
isDropdownOpen={isFavoriteFolderEditDropdownOpen}
/>
);

Expand Down
16 changes: 8 additions & 8 deletions ...nty-front/src/modules/favorites/components/FavoriteFolderNavigationDrawerItemDropdown.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,27 +1,24 @@
import { FavoriteFolderNavigationDrawerItemDropdownButton } from '@/favorites/components/FavoriteFolderNavigationDrawerItemDropdownButton';
import { FavoriteFolderHotkeyScope } from '@/favorites/constants/FavoriteFolderRightIconDropdownHotkeyScope';
import { Dropdown } from '@/ui/layout/dropdown/components/Dropdown';
import { DropdownMenuItemsContainer } from '@/ui/layout/dropdown/components/DropdownMenuItemsContainer';

import {
IconDotsVertical,
IconPencil,
IconTrash,
LightIconButton,
MenuItem,
} from 'twenty-ui';
import { IconPencil, IconTrash, MenuItem } from 'twenty-ui';

type FavoriteFolderNavigationDrawerItemDropdownProps = {
folderId: string;
onRename: () => void;
onDelete: () => void;
closeDropdown: () => void;
isDropdownOpen: boolean;
};

export const FavoriteFolderNavigationDrawerItemDropdown = ({
folderId,
onRename,
onDelete,
closeDropdown,
isDropdownOpen,
}: FavoriteFolderNavigationDrawerItemDropdownProps) => {
const handleRename = () => {
onRename();
Expand All @@ -41,7 +38,10 @@ export const FavoriteFolderNavigationDrawerItemDropdown = ({
}}
data-select-disable
clickableComponent={
<LightIconButton Icon={IconDotsVertical} accent="tertiary" />
Copy link
Member

Choose a reason for hiding this comment

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

we should keep using the LightIconButton here
I think it was a mistake to add a role=button in the Dropdown.tsx component on the container div of the clickableComponent.

Instead could we:

  1. Define a type for ClickableAccessibilityProps that will accept:
aria-controls: string
aria-expanded: boolean
aria-haspopup: boolean

  1. Extends LightIconButton props to add the ClickableAccessibilityProps

  2. Create a FavoriteFolderNavigationDrawerItemDropdownButton.tsx that will wrap the LightIconButton and provide the 3 props to it.

  3. Use this FavoriteFolderNavigationDrawerItemDropdownButton.tsx in FavoriteFolderNavigationDrawerItemDropdown.tsx

  4. Remove these props from the container in the Dropdown.tsx component

Copy link
Member

Choose a reason for hiding this comment

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

(and revert your changes here)

Copy link
Contributor

Choose a reason for hiding this comment

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

@Devessier aria-master WDYT ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for tagging me. I'm checking!

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's great to let the clickableComponent set these ARIA props. Currently, two elements with the button ARIA role are nested (the div in the Dropdown component, and the LightIconButton, which uses a button HTML element and has by default the button role). I think this is not correct.

https://github.com/twentyhq/twenty/pull/9560/files#diff-cde0fa86b1e8e890ce89cfb1eb0473ce7027963bc89f48168c8a885275f18f9aL137-L140

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm unsure if the ClickableAccessibilityProps name relates to the expected props. To me, aria-controls, aria-expanded, and aria-haspopup are more related to a popup/dropdown component.

These two components use these properties:

I'm unsure if the LightIconButton component will always be called in a way these three ARIA attributes must be set. The ARIA attributes to set depend on the context where the component is used. I think these properties should all be optional, and we could transform the ClickableAccessibilityProps type into a more generic AriaProps:

import { AriaAttributes, AriaRole } from 'react';

type AriaProps =
  | AriaAttributes
  | {
      role: AriaRole;
    };

I'm not sure about the right level of abstraction here. We should ensure it doesn't become a foot gun. The first lesson of any accessibility course is that using no ARIA attributes is better than misusing them, which can make the UX even worse for people using screen readers.

What do you think?

<FavoriteFolderNavigationDrawerItemDropdownButton
dropdownId={`favorite-folder-edit-${folderId}`}
Copy link
Contributor

Choose a reason for hiding this comment

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

style: dropdownId prop is redundant here since it's already being used on line 35

isDropdownOpen={isDropdownOpen}
/>
}
dropdownPlacement="bottom-start"
dropdownComponents={
Expand Down
22 changes: 22 additions & 0 deletions ...ont/src/modules/favorites/components/FavoriteFolderNavigationDrawerItemDropdownButton.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
import { LightIconButton } from '@ui/input/button/components/LightIconButton';
import { IconDotsVertical } from 'twenty-ui';

type FavoriteFolderNavigationDrawerItemDropdownButtonProps = {
dropdownId: string;
isDropdownOpen: boolean;
};

export const FavoriteFolderNavigationDrawerItemDropdownButton = ({
dropdownId,
isDropdownOpen,
}: FavoriteFolderNavigationDrawerItemDropdownButtonProps) => {
return (
<LightIconButton
Icon={IconDotsVertical}
accent="tertiary"
aria-controls={`${dropdownId}-options`}
aria-expanded={isDropdownOpen}
aria-haspopup={true}
/>
Comment on lines +14 to +20
Copy link
Contributor

Choose a reason for hiding this comment

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

logic: missing aria-label for screen readers to identify this button's purpose

Comment on lines +14 to +20
Copy link
Contributor

Choose a reason for hiding this comment

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

logic: missing onClick handler - button currently has no interaction capability

);
};
4 changes: 0 additions & 4 deletions packages/twenty-front/src/modules/ui/layout/dropdown/components/Dropdown.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,10 +134,6 @@ export const Dropdown = ({
<div
ref={refs.setReference}
onClick={handleClickableComponentClick}
aria-controls={`${dropdownId}-options`}
aria-expanded={isDropdownOpen}
aria-haspopup={true}
role="button"
>
Comment on lines 134 to 137
Copy link
Contributor

Choose a reason for hiding this comment

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

logic: Removing ARIA attributes breaks accessibility. The clickable div needs aria-controls, aria-expanded, aria-haspopup, and role='button' for screen reader support.

Suggested change
<div
ref={refs.setReference}
onClick={handleClickableComponentClick}
aria-controls={`${dropdownId}-options`}
aria-expanded={isDropdownOpen}
aria-haspopup={true}
role="button"
>
<div
ref={refs.setReference}
onClick={handleClickableComponentClick}
aria-controls={`${dropdownId}-options`}
aria-expanded={isDropdownOpen}
aria-haspopup={true}
role="button"
>

{clickableComponent}
</div>
Expand Down
2 changes: 2 additions & 0 deletions packages/twenty-ui/src/accessibility/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
export * from './components/VisibilityHidden';
export * from './components/VisibilityHiddenInput';
export * from './types/ClickableAccessibilityProps';
export * from './utils/visibility-hidden';

7 changes: 7 additions & 0 deletions packages/twenty-ui/src/accessibility/types/ClickableAccessibilityProps.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
export type ClickableAccessibilityProps = {
'aria-controls': string;

'aria-expanded': boolean;

'aria-haspopup': boolean;
Copy link
Contributor

Choose a reason for hiding this comment

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

logic: aria-haspopup should accept boolean | 'menu' | 'listbox' | 'tree' | 'grid' | 'dialog' according to ARIA spec

};
Comment on lines +1 to +7
Copy link
Contributor

Choose a reason for hiding this comment

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

style: Consider making these props optional with '?' since not all clickable elements will need all three ARIA attributes. Some buttons may only need aria-expanded without aria-controls.

4 changes: 3 additions & 1 deletion packages/twenty-ui/src/input/button/components/LightIconButton.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { useTheme } from '@emotion/react';
import styled from '@emotion/styled';
import { ClickableAccessibilityProps } from '@ui/accessibility';
import { IconComponent } from '@ui/display';
import { ComponentProps, MouseEvent } from 'react';

Expand All @@ -17,7 +18,8 @@ export type LightIconButtonProps = {
disabled?: boolean;
focus?: boolean;
onClick?: (event: MouseEvent<HTMLButtonElement>) => void;
} & Pick<ComponentProps<'button'>, 'aria-label' | 'title'>;
} & Pick<ComponentProps<'button'>, 'aria-label' | 'title'> &
Partial<ClickableAccessibilityProps>;
Comment on lines +21 to +22
Copy link
Contributor

Choose a reason for hiding this comment

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

logic: making ClickableAccessibilityProps partial means aria-controls, aria-expanded, and aria-haspopup will be optional, which could lead to incomplete accessibility implementation in consuming components


const StyledButton = styled.button<
Pick<LightIconButtonProps, 'accent' | 'active' | 'size' | 'focus'>
Expand Down
Loading