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

Improve and document the TableFilter component #163

Merged
merged 7 commits into from
Feb 20, 2025

Conversation

jaredoconnell
Copy link
Member

I found the TableFilter component confusing due to the duplicate input and lack of documentation. This PR aims to fix that.

  • Adds a notice when no filter options are present, or when no options are present for that filter.
  • Reduces duplicate inputs by utilizing existing input to get necessary information
  • Documents TableFilters and ColumnMenuFilter components

This was entirely undocumented before, so let me know if I have any documentation errors.

image

Type of change

  • Refactor
  • New feature
  • Bug fix
  • Optimization
  • Documentation Update

Description

Related Tickets & Documents

  • Related Issue #
  • Closes #

Checklist before requesting a review

  • I have performed a self-review of my code.
  • If it is a core feature, I have added thorough tests.

Testing

  • Please describe the System Under Test.
  • Please provide detailed steps to perform tests related to this code change.
  • How were the fix/results from this change verified? Please provide relevant screenshots or results.

Adds a notice when no filter options are present

Reduces duplicate inputs by utilizing existing input to get necessary information

Documents TableFilters and ColumnMenuFilter components
Copy link
Collaborator

@dbutenhof dbutenhof left a comment

Choose a reason for hiding this comment

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

First pass comments ... I may need to try to grok the component actions and reducers to really understand exactly what you've done by removing the filterData prop...

Adds documentation, and updates the message when no options are present
Copy link
Member Author

@jaredoconnell jaredoconnell left a comment

Choose a reason for hiding this comment

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

Addressed review comments. Also, I made improvements to MultiSelectBox.

Copy link
Collaborator

@dbutenhof dbutenhof left a comment

Choose a reason for hiding this comment

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

But I think Varshini's comments are worth addressing.

Copy link
Collaborator

@MVarshini MVarshini left a comment

Choose a reason for hiding this comment

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

LGTM!

@jaredoconnell jaredoconnell merged commit c2c7dcb into revamp Feb 20, 2025
1 check passed
@jaredoconnell jaredoconnell deleted the improve-tablefilter branch February 20, 2025 14:29
@@ -3,6 +3,7 @@ import "react-calendar/dist/Calendar.css";
import "./index.less";

import {
Banner,
Copy link
Collaborator

Choose a reason for hiding this comment

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

You added the Banner import, but I don't see it being used. Ah, well. I see you merged, so someone can clean it up later. I'll bet ESLint will catch it.

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.

3 participants