vibe icon indicating copy to clipboard operation
vibe copied to clipboard

feat(Dropdown): New dropdown component - WIP

Open rivka-ungar opened this issue 9 months ago โ€ข 1 comments

https://monday.monday.com/boards/3532714909/views/80492480/pulses/9196812270

rivka-ungar avatar May 20 '25 23:05 rivka-ungar

PR Reviewer Guide ๐Ÿ”

(Review updated until commit https://github.com/mondaycom/vibe/commit/96f4e43faa5d5f05c218d8c66b4ed33b0c297b19)

Here are some key observations to aid the review process:

โฑ๏ธย Estimated effort to review: 5 ๐Ÿ”ต๐Ÿ”ต๐Ÿ”ต๐Ÿ”ต๐Ÿ”ต
๐Ÿงชย PR contains tests
๐Ÿ”’ย No security concerns identified
โšกย Recommended focus areas for review

Breaking Change

The default behavior for searchable has changed from true to false, which is a significant breaking change that could affect existing implementations. This needs careful validation to ensure it's intentional and properly documented.

const isSearchable = Boolean(dropdownProps.searchable);

Logic Error

The onIsOpenChange callback logic appears to be inverted - it calls onMenuOpen when isOpen is true, but typically you'd want to call onMenuOpen when the menu opens (isOpen becomes true).

  isOpen ? onMenuOpen?.() : onMenuClose?.();
}
Event Handling

Multiple event handlers are stopping propagation and preventing default behavior which could interfere with accessibility features like keyboard navigation and screen readers.

<div
  onClick={e => {
    e.stopPropagation();
  }}
  onKeyDown={e => {
    e.stopPropagation();
  }}
  onMouseDown={e => {
    e.stopPropagation();
  }}
>
  <Dialog
    content={dialogContent}
    showTrigger={["click", "enter"]}
    hideTrigger={["clickoutside", "enter"]}
    position="bottom"
    moveBy={{ main: 4 }}
    hideWhenReferenceHidden
    addKeyboardHideShowTriggersByDefault
  >
    <Chips
      label={`+ ${hiddenCount}`}
      readOnly
      noMargin
      ariaLabel={`+${hiddenCount}. ${selectedItems.length - hiddenCount} items are visible out of ${
        selectedItems.length
      }`}
      data-testid="dropdown-overflow-counter"
      className={styles.overflowCounter}
      onClick={() => {}}
      onMouseDown={e => {
        e.preventDefault();
      }}
    />
  </Dialog>
</div>