Skip to content

Conversation

@hoonji
Copy link
Member

@hoonji hoonji commented Mar 15, 2024

Motivation for features / changes

Splits DataTableComponent's context menu functionality into its own component to be compatible with future updates that will enable dynamic custom modal creation (for alleviating stacking context issues)

Technical description of changes

  • No new functionality - simply moves the context menu related template and logic to a new ContextMenuComponent
  • Parameterizes some context menu tests for legibility.

Detailed steps to verify changes work correctly (as executed by you)

  • Tested manually that all context menu features work as before

Copy link
Member Author

Choose a reason for hiding this comment

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

copied from data_table_component.ng.html

Copy link
Member Author

Choose a reason for hiding this comment

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

Copied from data_table_component.scss

Copy link
Member Author

Choose a reason for hiding this comment

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

Copied from data_table_component.ts

Copy link
Member Author

@hoonji hoonji Mar 15, 2024

Choose a reason for hiding this comment

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

Mostly copied from data_table_test.ts with some tests changed to be parametrized

mat-icon-button
class="add-column-btn"
(click)="openColumnSelector($event)"
(click)="openColumnSelector({event: $event})"
Copy link
Member Author

Choose a reason for hiding this comment

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

To allow ContextMenuComponent to propagate openColumnSelector parameters through its output, we need to make openColumnSelector's parameters an object as an Event can only be an object.

});
}

openFilterMenu(event: MouseEvent, header: ColumnHeader) {
Copy link
Member Author

Choose a reason for hiding this comment

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

header is exactly equivalent to this.contextMenuHeader

:host {
display: flex;
font-size: 13px;
min-width: 210px;
Copy link
Member Author

Choose a reason for hiding this comment

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

Fixes discrete filter placeholder text from being cut off:
6tXMnVtA3ELS7c2

@hoonji hoonji requested a review from bmd3k March 15, 2024 12:15
return index;
}

onColumnAdded(header: ColumnHeader) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I played with your changes. "Insert Column Left" no longer works as expected. It inserts to the right instead.

Copy link
Member Author

@hoonji hoonji Mar 18, 2024

Choose a reason for hiding this comment

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

Oh thanks for catching this! To clarify, the quoted "getInsertIndex()" code should be irrelevant as it's unused, but the other changes introduced a race condition where insertColumnTo is reset to undefined after it is first properly set on column open.

Added a temporary fix (namely don't reset insertColumnTo to undefined - this is actually unnecessary) and included a simple regression test. Note that this class of problem should be irrelevant in the next PR that creates modals dynamically.

@hoonji hoonji merged commit db29bcc into tensorflow:master Mar 18, 2024
@hoonji hoonji deleted the context_menu_fix branch March 18, 2024 08:08
AnuarTB pushed a commit to AnuarTB/tensorboard that referenced this pull request May 3, 2024
## Motivation for features / changes
Splits DataTableComponent's context menu functionality into its own
component to be compatible with future updates that will enable dynamic
custom modal creation (for alleviating [stacking context
issues](tensorflow#6737 (comment)))

## Technical description of changes
- No new functionality - simply moves the context menu related template
and logic to a new ContextMenuComponent
- Parameterizes some context menu tests for legibility.

## Detailed steps to verify changes work correctly (as executed by you)
- Tested manually that all context menu features work as before
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.

2 participants