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

feat: add CheckboxTree component #1495

Merged
merged 65 commits into from
Jan 21, 2025
Merged

Conversation

rohanm-crest
Copy link
Contributor

@rohanm-crest rohanm-crest commented Nov 29, 2024

Issue number: ADDON-76198

PR Type

What kind of change does this PR introduce?

  • Feature
  • Bug Fix
  • Refactoring (no functional or API changes)
  • Documentation Update
  • Maintenance (dependency updates, CI, etc.)

Summary

Changes

A new component CheckboxTree has been introduced to define hierarchical checkbox structures with support of search functionality.

User experience

User can use the new component for handling the list of checkboxes with better UI including the expanding/collapsing, grouping and searching within the checkbox list.

Screenshot 2024-12-13 at 3 39 25 PM

Checklist

If an item doesn't apply to your changes, leave it unchecked.

@rohanm-crest rohanm-crest changed the title feat: introduce the new CheckboxTree component feat: introduce the new checkboxtree component Nov 29, 2024
@rohanm-crest rohanm-crest force-pushed the feat/checkbox-tree-component branch from 7bacfa3 to f23a4c0 Compare December 4, 2024 06:20
@rohanm-crest rohanm-crest self-assigned this Dec 6, 2024
@rohanm-crest rohanm-crest marked this pull request as ready for review December 9, 2024 06:21
@rohanm-crest rohanm-crest requested review from a team as code owners December 9, 2024 06:21
Copy link
Contributor

@soleksy-splunk soleksy-splunk left a comment

Choose a reason for hiding this comment

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

still reviewing but will post just after adding, feel free to close if something seems like a bad/worse idea

ui/src/components/CheckboxTree/CheckboxTree.test.tsx Outdated Show resolved Hide resolved
ui/src/components/CheckboxTree/CheckboxTree.utils.ts Outdated Show resolved Hide resolved
Signed-off-by: Viktor Tsvetkov <[email protected]>
Copy link
Contributor

@soleksy-splunk soleksy-splunk left a comment

Choose a reason for hiding this comment

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

Some additional ideas/questions.

Generally looks good to me.

Seems like Viktor got a good point regarding those css-s, it might be useful to get rid of as much as we can. (I would say worth removing even if it will make feature looks a bit worse)

ui/src/components/CheckboxTree/CheckboxTree.utils.ts Outdated Show resolved Hide resolved
ui/src/components/CheckboxTree/types.ts Outdated Show resolved Hide resolved
ui/src/components/CheckboxTree/types.ts Show resolved Hide resolved
Signed-off-by: Viktor Tsvetkov <[email protected]>
@vtsvetkov-splunk
Copy link
Contributor

Suggested pull request title: feat: add CheckboxTree component for hierarchical checkbox management

Great work on implementing a new CheckboxTree component that provides a hierarchical structure for managing checkboxes! The implementation looks clean and well-organized, with proper testing and documentation in place.

I especially appreciate:

  • The comprehensive unit test coverage with test cases covering various scenarios
  • Clear documentation including examples in both markdown and Storybook
  • Well-structured component architecture with separation of concerns
  • Proper handling of accessibility through ARIA labels
  • Support for both expandable and non-expandable groups
  • Implementation of "Select All" and "Clear All" functionality

Here are a few suggestions for improvements:

  1. Documentation:
  • Add a brief section in the component documentation about accessibility features
  • Include examples of keyboard navigation in the docs
  1. Code Structure:
  • Consider extracting some of the complex state management logic from CheckboxTree.tsx into custom hooks for better reusability
  • The CheckboxSubTree component could potentially be split further if it grows more complex
  1. Testing:
  • Add more test cases for keyboard navigation
  • Consider adding tests for various error states and edge cases
  • Add integration tests for interaction between groups and individual checkboxes
  1. Accessibility:
  • Consider adding keyboard shortcuts for bulk actions (Select All/Clear All)
  • Ensure proper focus management when expanding/collapsing groups
  1. Performance:
  • Consider memoizing more of the callback functions using useCallback
  • Look into potential performance optimizations for large checkbox trees

Overall, this is a solid implementation that appears ready for merging after addressing these minor suggestions. The component will be a valuable addition to the UI framework.

This comment was added by our PR Review Assistant Bot. Please kindly acknowledge that
while we're doing our best to keep these comments up to very high standards, they may occasionally be
incorrect. Suggestions offered by the Bot are only intended as points for consideration and no statements
by this bot alone can be considered grounds for merging of any pull request. Remember to seek a review
from a human co-worker.

To reply to the review and engage Review Bot in further conversation, start your comment with the words Review bot:

Copy link
Contributor

@vtsvetkov-splunk vtsvetkov-splunk left a comment

Choose a reason for hiding this comment

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

LGTM, great work

@rohanm-crest rohanm-crest merged commit 1d55507 into develop Jan 21, 2025
122 checks passed
@rohanm-crest rohanm-crest deleted the feat/checkbox-tree-component branch January 21, 2025 13:18
@github-actions github-actions bot locked and limited conversation to collaborators Jan 21, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants