Skip to content

Conversation

@platypii
Copy link
Contributor

@platypii platypii commented Apr 7, 2025

Adds a Dropdown component to the hyperparam package. Useful for the demos, etc.

I also fixed a few random nits and upgraded the dependencies.

@platypii platypii requested review from bleakley and severo April 7, 2025 03:22
@severo severo mentioned this pull request Apr 7, 2025
Copy link
Contributor

@severo severo left a comment

Choose a reason for hiding this comment

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

cool. Some comments + I made a PR against this branch: #207

@platypii
Copy link
Contributor Author

platypii commented Apr 8, 2025

@bleakley

@bleakley
Copy link
Contributor

bleakley commented Apr 8, 2025

We should also add a keyboard handler here for up/down arrow. Going down from the main button should loop through the children but not cycle back to the main button again.

function handleArrowKeys(event: KeyboardEvent) {
      if (isOpen && (event.key === 'ArrowDown' || event.key === 'ArrowUp')) {
        event.preventDefault()
        const focusableElements = menuRef.current?.querySelectorAll<HTMLElement>(
          'button, [href], input, select, textarea, [tabindex]:not([tabindex="-1"])'
        )
        if (focusableElements && focusableElements.length > 0) {
          const currentIndex = Array.from(focusableElements).indexOf(document.activeElement as HTMLElement)
          let nextIndex = currentIndex
          if (event.key === 'ArrowDown') {
            nextIndex = (currentIndex + 1) % focusableElements.length
          } else if (event.key === 'ArrowUp') {
            nextIndex = (currentIndex - 1 + focusableElements.length) % focusableElements.length
          }
          focusableElements[nextIndex]?.focus()
        }
      }
    }

Copy link
Contributor

@bleakley bleakley left a comment

Choose a reason for hiding this comment

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

Arrow key handling would be nice but not absolutely needed

@platypii platypii requested a review from bleakley April 10, 2025 21:59
}
}

function handleEscape(event: KeyboardEvent) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need to add this escape key listener to the dropdown anymore, unless there's some tiny part of the element that's outside of the button and the content.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if the user tabs out to another part of the page and hits escape, we should probably close the menu? leaving it for now

@platypii platypii merged commit 167f82e into master Apr 11, 2025
4 checks passed
@platypii platypii deleted the dropdown branch April 11, 2025 07:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants