minor fix -- console logs #9560

ehconitin · 2025-01-11T09:25:42Z

greptile-apps

PR Summary

Simplified dropdown trigger implementation in FavoriteFolderNavigationDrawerItemDropdown by replacing LightIconButton with a custom styled container.

Replaced LightIconButton with StyledIconContainer in /packages/twenty-front/src/modules/favorites/components/FavoriteFolderNavigationDrawerItemDropdown.tsx for more direct theme control
Added theme-based border radius and transparent background styling to maintain consistent UI appearance
Removed unused button component imports to reduce bundle size

_{1 file(s) reviewed, 1 comment(s)}
_{Edit PR Review Bot Settings | Greptile}

...twenty-front/src/modules/favorites/components/FavoriteFolderNavigationDrawerItemDropdown.tsx

charlesBochet · 2025-01-11T09:57:37Z

...twenty-front/src/modules/favorites/components/FavoriteFolderNavigationDrawerItemDropdown.tsx

@@ -41,7 +49,12 @@ export const FavoriteFolderNavigationDrawerItemDropdown = ({
      }}
      data-select-disable
      clickableComponent={
-        <LightIconButton Icon={IconDotsVertical} accent="tertiary" />


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:

Define a type for ClickableAccessibilityProps that will accept:

aria-controls: string aria-expanded: boolean aria-haspopup: boolean

Extends LightIconButton props to add the ClickableAccessibilityProps

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

Use this FavoriteFolderNavigationDrawerItemDropdownButton.tsx in FavoriteFolderNavigationDrawerItemDropdown.tsx

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

(and revert your changes here)

@Devessier aria-master WDYT ?

Thanks for tagging me. I'm checking!

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

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:

https://headlessui.com/react/menu

https://headlessui.com/react/popover

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?

charlesBochet

Thanks @ehconitin
I think we should go for a different approach, see my comment

charlesBochet · 2025-01-13T09:34:27Z

@ehconitin I'm closing this PR for now, re-open it once ready :)

greptile-apps

PR Summary

(updates since last review)

This PR reorders CSS properties in the StyledIconContainer component, moving 'display: flex' before 'justify-content: center' for better code organization.

Reordered CSS properties in /packages/twenty-front/src/modules/favorites/components/FavoriteFolderNavigationDrawerItemDropdown.tsx for improved readability

Note: Since this is a minor cosmetic change to CSS property ordering with no functional impact, and the previous review already covered the major component changes, there are no other significant points to highlight.

_{1 file(s) reviewed, no comment(s)}
_{Edit PR Review Bot Settings | Greptile}

ehconitin · 2025-01-24T12:33:35Z

Hi @charlesBochet - just want to clear something up. I noticed you're focused on keeping LightIconButton because of design consistency (which makes sense), but I think you might have missed my earlier point about NavigationDrawerItem being a button itself.
When you look at where this dropdown lives, it's actually nested inside NavigationDrawerItem which renders as a . This means even if we use LightIconButton (which is also a ), we'll still get that nested button error in the console - it's not really about the Dropdown container div's aria attributes. However if you think we should get rid of dropdowns aria attribute I have tried your approach in latest changes --- but it doesn't solve the core issue of this PR

greptile-apps

PR Summary

(updates since last review)

This PR removes essential ARIA accessibility attributes from the Dropdown component while introducing new accessibility-related types and structures.

Removed critical ARIA attributes (aria-controls, aria-expanded, aria-haspopup) from clickable div in /packages/twenty-front/src/modules/ui/layout/dropdown/components/Dropdown.tsx
Added new ClickableAccessibilityProps type in /packages/twenty-ui/src/accessibility/types/ClickableAccessibilityProps.ts for standardizing accessibility props
Extended LightIconButton in /packages/twenty-ui/src/input/button/components/LightIconButton.tsx to support ARIA attributes through Partial<ClickableAccessibilityProps>

Note: The removal of ARIA attributes from the Dropdown component should be reverted to maintain accessibility standards.

_{7 file(s) reviewed, 7 comment(s)}
_{Edit PR Review Bot Settings | Greptile}

greptile-apps · 2025-01-24T12:34:32Z

packages/twenty-front/src/modules/ui/layout/dropdown/components/Dropdown.tsx

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


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"

>

greptile-apps · 2025-01-24T12:34:35Z

...-front/src/modules/favorites/components/FavoriteFolderNavigationDrawerItemDropdownButton.tsx

+    <LightIconButton
+      Icon={IconDotsVertical}
+      accent="tertiary"
+      aria-controls={`${dropdownId}-options`}
+      aria-expanded={isDropdownOpen}
+      aria-haspopup={true}
+    />


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

greptile-apps · 2025-01-24T12:34:36Z

...-front/src/modules/favorites/components/FavoriteFolderNavigationDrawerItemDropdownButton.tsx

+    <LightIconButton
+      Icon={IconDotsVertical}
+      accent="tertiary"
+      aria-controls={`${dropdownId}-options`}
+      aria-expanded={isDropdownOpen}
+      aria-haspopup={true}
+    />


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

greptile-apps · 2025-01-24T12:34:38Z

...twenty-front/src/modules/favorites/components/FavoriteFolderNavigationDrawerItemDropdown.tsx

@@ -41,7 +38,10 @@ export const FavoriteFolderNavigationDrawerItemDropdown = ({
      }}
      data-select-disable
      clickableComponent={
-        <LightIconButton Icon={IconDotsVertical} accent="tertiary" />
+        <FavoriteFolderNavigationDrawerItemDropdownButton
+          dropdownId={`favorite-folder-edit-${folderId}`}


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

greptile-apps · 2025-01-24T12:35:29Z

packages/twenty-ui/src/accessibility/types/ClickableAccessibilityProps.ts

+export type ClickableAccessibilityProps = {
+  'aria-controls': string;
+
+  'aria-expanded': boolean;
+
+  'aria-haspopup': boolean;
+};


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.

greptile-apps · 2025-01-24T12:35:29Z

packages/twenty-ui/src/accessibility/types/ClickableAccessibilityProps.ts

+
+  'aria-expanded': boolean;
+
+  'aria-haspopup': boolean;


greptile-apps · 2025-01-24T12:35:37Z

packages/twenty-ui/src/input/button/components/LightIconButton.tsx

+} & Pick<ComponentProps<'button'>, 'aria-label' | 'title'> &
+  Partial<ClickableAccessibilityProps>;


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

charlesBochet · 2025-01-24T14:05:10Z

Hi @charlesBochet - just want to clear something up. I noticed you're focused on keeping LightIconButton because of design consistency (which makes sense), but I think you might have missed my earlier point about NavigationDrawerItem being a button itself. When you look at where this dropdown lives, it's actually nested inside NavigationDrawerItem which renders as a . This means even if we use LightIconButton (which is also a ), we'll still get that nested button error in the console - it's not really about the Dropdown container div's aria attributes. However if you think we should get rid of dropdowns aria attribute I have tried your approach in latest changes --- but it doesn't solve the core issue of this PR

I still think #9560 (comment) is the right approach, why don't it work?

Edit: you actually implemented it, I'm taking a deep look tonight

fix

c8e0157

ehconitin requested a review from FelixMalfait January 11, 2025 09:25

greptile-apps bot reviewed Jan 11, 2025

View reviewed changes

...twenty-front/src/modules/favorites/components/FavoriteFolderNavigationDrawerItemDropdown.tsx Outdated Show resolved Hide resolved

grep

5a1efd7

charlesBochet reviewed Jan 11, 2025

View reviewed changes

charlesBochet requested changes Jan 11, 2025

View reviewed changes

charlesBochet added the -PR: awaiting author label Jan 11, 2025

charlesBochet assigned ehconitin Jan 11, 2025

ehconitin added the -PR: wip label Jan 11, 2025

charlesBochet closed this Jan 13, 2025

ehconitin reopened this Jan 24, 2025

greptile-apps bot reviewed Jan 24, 2025

View reviewed changes

ehconitin marked this pull request as draft January 24, 2025 11:57

ehconitin and others added 2 commits January 24, 2025 17:28

Merge branch 'main' into console-logs-fix

86ab356

charles review

0878d42

ehconitin marked this pull request as ready for review January 24, 2025 12:33

ehconitin requested a review from charlesBochet January 24, 2025 12:33

ehconitin added -PR: awaiting review and removed -PR: wip -PR: awaiting author labels Jan 24, 2025

greptile-apps bot reviewed Jan 24, 2025

View reviewed changes

lucasbordeau requested a review from Devessier January 30, 2025 16:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

minor fix -- console logs #9560

minor fix -- console logs #9560

ehconitin commented Jan 11, 2025

greptile-apps bot left a comment

charlesBochet Jan 11, 2025

charlesBochet Jan 11, 2025

lucasbordeau Jan 30, 2025

Devessier Jan 30, 2025

Devessier Jan 30, 2025

Devessier Jan 30, 2025

charlesBochet left a comment

charlesBochet commented Jan 13, 2025

greptile-apps bot left a comment

ehconitin commented Jan 24, 2025

greptile-apps bot left a comment

greptile-apps bot Jan 24, 2025

greptile-apps bot Jan 24, 2025

greptile-apps bot Jan 24, 2025

greptile-apps bot Jan 24, 2025

greptile-apps bot Jan 24, 2025

greptile-apps bot Jan 24, 2025

greptile-apps bot Jan 24, 2025

charlesBochet commented Jan 24, 2025 •

edited

Loading

		} & Pick<ComponentProps<'button'>, 'aria-label' \| 'title'> &
		Partial<ClickableAccessibilityProps>;

minor fix -- console logs #9560

Are you sure you want to change the base?

minor fix -- console logs #9560

Conversation

ehconitin commented Jan 11, 2025

greptile-apps bot left a comment

Choose a reason for hiding this comment

PR Summary

charlesBochet Jan 11, 2025

Choose a reason for hiding this comment

charlesBochet Jan 11, 2025

Choose a reason for hiding this comment

lucasbordeau Jan 30, 2025

Choose a reason for hiding this comment

Devessier Jan 30, 2025

Choose a reason for hiding this comment

Devessier Jan 30, 2025

Choose a reason for hiding this comment

Devessier Jan 30, 2025

Choose a reason for hiding this comment

charlesBochet left a comment

Choose a reason for hiding this comment

charlesBochet commented Jan 13, 2025

greptile-apps bot left a comment

Choose a reason for hiding this comment

PR Summary

ehconitin commented Jan 24, 2025

greptile-apps bot left a comment

Choose a reason for hiding this comment

PR Summary

greptile-apps bot Jan 24, 2025

Choose a reason for hiding this comment

greptile-apps bot Jan 24, 2025

Choose a reason for hiding this comment

greptile-apps bot Jan 24, 2025

Choose a reason for hiding this comment

greptile-apps bot Jan 24, 2025

Choose a reason for hiding this comment

greptile-apps bot Jan 24, 2025

Choose a reason for hiding this comment

greptile-apps bot Jan 24, 2025

Choose a reason for hiding this comment

greptile-apps bot Jan 24, 2025

Choose a reason for hiding this comment

charlesBochet commented Jan 24, 2025 • edited Loading

charlesBochet commented Jan 24, 2025 •

edited

Loading