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.

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

fix: improve keyboard navigation and event handling for logs columns popup #6599

Closed
wants to merge 5 commits into from

Conversation

ahmadshaheer
Copy link
Collaborator

@ahmadshaheer ahmadshaheer commented Dec 5, 2024

Summary

  • Replace window.addEventListener with React's onKeyDown handler
  • Add arrow key navigation to select first/last dropdown options if there is no selected option
  • Fix edge cases in option selection logic (don't allow selection options if options array is empty)

Related Issues / PR's

Fixes this Sentry

Screenshots

Before:

2024-12-05.12-32-30.mov

After:

2024-12-05.13-59-12.mov

Affected Areas and Manually Tested Areas

  • Add new column popup for logs
  • Tested
    • Pressing enter when no option is selected, should not break the page
    • If no option is selected, pressing up arrow should select the last item
    • If no option is selected, pressing down arrow should select the first item
    • Pressing enter should add the column

Important

Improves keyboard navigation and event handling in LogsFormatOptionsMenu by using onKeyDown and handling edge cases.

  • Behavior:
    • Replaces window.addEventListener with onKeyDown handler in LogsFormatOptionsMenu.tsx for keyboard events.
    • Adds arrow key navigation to select first/last dropdown options if no option is selected.
    • Prevents option selection if options array is empty.
  • Styles:
    • Adds focus-visible style to .add-new-column-container in LogsFormatOptionsMenu.styles.scss.

This description was created by Ellipsis for f76c505. It will automatically update as commits are pushed.

…t handling

- Replace window.addEventListener with React's onKeyDown handler
- Add arrow key navigation to select first/last dropdown options if there is no selected option
- Fix edge cases in option selection logic (don't allow selection options if options array is empty)
- Add focus-visible styles to the new column container
- Update the new column container to include role and tabIndex for better keyboard navigation
…n is selected

- Add a condition to ignore Enter key presses if no option is currently selected
@ahmadshaheer ahmadshaheer requested a review from YounixM as a code owner December 5, 2024 09:54
Copy link

github-actions bot commented Dec 5, 2024

Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id>

@github-actions github-actions bot added the bug Something isn't working label Dec 5, 2024
Copy link

github-actions bot commented Dec 5, 2024

Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id>

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me! Reviewed everything up to f76c505 in 29 seconds

More details
  • Looked at 211 lines of code in 2 files
  • Skipped 0 files when reviewing.
  • Skipped posting 4 drafted comments based on config settings.
1. frontend/src/components/LogsFormatOptionsMenu/LogsFormatOptionsMenu.tsx:142
  • Draft comment:
    Ensure that the dependencies for useCallback hooks are accurate. The current implementation seems correct, but double-check that addColumn and selectedValue are the only dependencies needed for handleColumnSelection, and addColumn?.options, selectedValue, and handleColumnSelection for handleKeyDown.
  • Reason this comment was not posted:
    Confidence changes required: 20%
    The use of useCallback for handleColumnSelection and handleKeyDown is appropriate, but the dependencies should be checked. The handleColumnSelection function uses addColumn and selectedValue, so these should be in the dependency array. The handleKeyDown function uses addColumn?.options, selectedValue, and handleColumnSelection, so these should be in the dependency array. The current implementation seems correct, but it's good to ensure that these dependencies are accurate.
2. frontend/src/components/LogsFormatOptionsMenu/LogsFormatOptionsMenu.tsx:158
  • Draft comment:
    Consider making the logic for handling the Enter key when no option is selected more explicit. Currently, it returns early, but explicitly stating that no action is taken might improve readability.
  • Reason this comment was not posted:
    Confidence changes required: 10%
    The handleKeyDown function is correctly using e.preventDefault() to prevent default behavior for certain keys, which is good practice. However, the logic for handling the Enter key when no option is selected could be more explicit. Currently, it returns early, but it might be clearer to explicitly state that no action is taken when Enter is pressed without a selection.
3. frontend/src/components/LogsFormatOptionsMenu/LogsFormatOptionsMenu.styles.scss:651
  • Draft comment:
    Use design tokens or predefined color constants instead of hardcoding color values like #1d212d to maintain consistency in design and theming. This is also applicable to other hardcoded color values in this file.
  • Reason this comment was not posted:
    Comment was not on a valid diff hunk.
4. frontend/src/components/LogsFormatOptionsMenu/LogsFormatOptionsMenu.tsx:283
  • Draft comment:
    Avoid using inline styles in React components. Instead, use external stylesheets, CSS classes, or styled components. This is also applicable to other inline styles in this file.
  • Reason this comment was not posted:
    Comment was not on a valid diff hunk.

Workflow ID: wflow_lgz5X4pKLKLfugMK


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

@ahmadshaheer ahmadshaheer changed the title Fix/logs new column keyboard nav fix: improve keyboard navigation and event handling for logs columns popup Dec 5, 2024
Copy link

github-actions bot commented Dec 5, 2024

Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id>

@ahmadshaheer ahmadshaheer enabled auto-merge (squash) December 18, 2024 08:15
@pranay01 pranay01 deleted the branch develop December 19, 2024 13:03
@pranay01 pranay01 closed this Dec 19, 2024
auto-merge was automatically disabled December 19, 2024 13:03

Pull request was closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants