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

Emit event on table group open/collapse #2133 #2402

Merged
merged 2 commits into from
Oct 2, 2024

Conversation

dbranley
Copy link
Contributor

@dbranley dbranley commented Sep 30, 2024

The PR fulfills these requirements: (check all the apply)

  • It's submitted to the main branch.
  • When resolving a specific issue, it's referenced in the PR's title (e.g. feat: Add a button #xxx, where "xxx" is the issue number).
  • When resolving a specific issue, the PR description includes Closes #xxx, where "xxx" is the issue number.
  • If changes were made to ui folder, unit tests (make test) still pass.
  • New/updated tests are included

Closes #2133

As requested in the ticket, I have made changes so that the table component will emit an event called group_change whenever a group is collapsed or opened. The group_change event data sent is a list of the group names that had their collapse state changed. The expectation is that the client py code will use this as a toggle to keep track of the collapsed states when the table has to be updated.

The change for this is in by both the onToggleCollapseAll and onToggleCollapse functions in table.tsx.

But before changing this, I had to understand expandedRefs.current. When set to null, it means all the groups are currently open. And when it is not null, then it means all the groups are collapsed, except for the groups whose names are in the expandedRefs.current dictionary with a value of false. So if you imagine a table with 2 groups called 'Bob' and 'John', here is what various definitions of expandedRefs.current look like:

  • null - all groups open
  • {'Bob': false} - 'Bob' is open and 'John' is collapsed
  • {} - all groups collapsed

Here's an explanation of the logic changes in the 2 functions:

onToggleCollapseAll:

When collapsing all the groups:

  • if expandedRefs.current is null, then it means all the groups are now open. Therefore, all the group names will be included in the event.
  • if expandedRefs.current is not null, then it means only the groups in the expandedRefs.current dict are open. Therefore, the group names in the dict will be included in the event.

When opening all the groups:

  • this can only happen when all the groups are collapsed. Therefore, all the group names will be included in the event.

onToggleCollapse:

The event from here will always just be the group name whose collapse state is changing.

While testing this, I found a bug which I had to fix. The issue was in the else-block in the code that was updating expandedRefs.current:

        if (expandedRefs.current) {
          isCollapsed
            ? expandedRefs.current[key] = false
            : delete expandedRefs.current[key]
        } else {
          expandedRefs.current = { [key]: false }
        }

As I mentioned above, when expandedRefs.current is null, that means all the groups are currently open and the user must be closing the current group. But the code in the else-block is setting expandedRefs.current to { 'group_name': false }, which is wrong. In order to properly reflect that 1 group is collapsed while all the others are open, the dict needs to contain all those other groups with a value of false. I made that fix and also added a new unit test that replicated the steps to reproduce the bug - Collapses all groups when some already collapsed - fire event.

I updated the documentation of the events prop to mention the new event type available to listen for. I also updated the text a bit since before it suggested that all events depended on pagination being set, but both select and now group_change are sent regardless of pagination.

  /** The events to capture on this table. When pagination is set, one of 'search' | 'sort' | 'filter' | 'download' | 'page_change' | 'reset'. These events are available regardless of pagination: 'select' | 'group_change'. */
  events?: S[]

I then ran make generate to update/generate the corresponding py code to reflect the prop change.

I created several new unit tests in table.test.tsx to test this change both with predefined groups and also when group by is used. I also ran the entire test suite and everything passes fine:

issue2133_unit-tests

I also created a new py example called table_events_group.py to test the changes. It has a table with 2 predefined groups and is listening on the new group_change event. The events are used to maintain the collapse state for the groups so that the collapsed state is maintained when the table is updated. There is also a button to kick off a background task to add new rows to the table. Using this, you can see how the collapsed state is maintained as new rows are added to the table. Here's a short video of the new example:

issue2133_ui_test1.mov

I decided to include this new example in the PR and you can see it is reflected in my local build of the wave website:

issue2133_doc_example

I also locally modified the existing py table example to listen on the new event so that I can test it working for Group By groups. I did not include this in the PR. Here's a short video of that:

issue2133_ui_test2.mov

Finally, I updated the table.md widget documentation to mention the new event. I added a this to the existing section called With collapsed groups. You can see a screen shot of that here:

issue2133_doc_widget

Let me know if you have any questions or would like me to make any changes. Thanks!

Copy link
Collaborator

@mturoci mturoci left a comment

Choose a reason for hiding this comment

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

Phenomenal job @dbranley! Looks good overall, just a few small comments.

ui/src/table.tsx Outdated Show resolved Hide resolved
ui/src/table.test.tsx Show resolved Hide resolved
website/widgets/form/table.md Outdated Show resolved Hide resolved
py/examples/table_events_group.py Show resolved Hide resolved
…implify py example and generate new snapshot, doc edits h2oai#2133
@dbranley
Copy link
Contributor Author

dbranley commented Oct 1, 2024

@mturoci Thanks for the great feedback! I've made all the changes you suggested, although I kept the unit test as-is for now. See my in-line response to your suggestion and let me know what you think.

I re-ran all the unit tests and everything still passes. I then re-tested with the new example and all good. I also rebuilt the docs and reviewed those too.

Thanks!

Copy link
Collaborator

@mturoci mturoci left a comment

Choose a reason for hiding this comment

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

Thanks for a quick turnaround @dbranley!

@mturoci mturoci merged commit 5f83b36 into h2oai:main Oct 2, 2024
@dbranley dbranley deleted the feat/issue-2133 branch October 2, 2024 18: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.

Emit event on table group open/collapse
2 participants