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: ui.table context menu items #522

Merged
merged 22 commits into from
Jun 28, 2024

Conversation

mattrunyon
Copy link
Collaborator

@mattrunyon mattrunyon commented Jun 5, 2024

Fixes #321

Use my branch of web-client-ui to test. The necessary changes are in an external package for the plugin, so don't need to do anything special w/ the plugins. deephaven/web-client-ui#2083

Here's an example with a context menu item on the row and a sub-menu action item on the header

from deephaven import empty_table, time_table, ui

@ui.component
def my_comp():
    t = time_table("PT1S").update(["X=i", "Y=i"])

    return ui.table(
        t,
        context_items=[{ "title": "Test", "action": lambda d: print(d)}],
        context_column_header_items=[
            {
                "title": "Header",
                "actions": [{ "title": "Header-sub", "action": lambda d: print(d)}]
            }
        ]
    )

c = my_comp()

@mattrunyon mattrunyon self-assigned this Jun 5, 2024
plugins/ui/DESIGN.md Outdated Show resolved Hide resolved
plugins/ui/DESIGN.md Outdated Show resolved Hide resolved
plugins/ui/DESIGN.md Show resolved Hide resolved
@mattrunyon
Copy link
Collaborator Author

I'm wondering if we should even have context_row_header_items in the spec. We don't currently have row headers in iris-grid, and if we did they would probably be row numbers. Those row numbers are meaningless to the server unless we have keyed rows and use the key as the row header

It's something that wouldn't be hard to add to UITable if we added support/reason for it in iris-grid. But right now there is no reason for it

@mattrunyon mattrunyon marked this pull request as ready for review June 10, 2024 13:49
@dsmmcken
Copy link
Contributor

I feel strongly that the word "context menu" must appear in the name.

context_actions or context_items are not things. It's not just a "context", it's a "context menu". There's no such thing as context having meaning without the word menu beside it. It's a compound noun.

It's like ice cream. You wouldn't label something as ice cone. That's not a thing, it's an ice cream cone. It's a compound noun.

@mattrunyon
Copy link
Collaborator Author

context_menu_items and column_header_context_menu_items? context_menu_column_header_items to me sounds like the context menu has some column_header that I can put items in

Another option would be just context_menu_items and then a dict w/ keys cell and column_header and values for the items, but I think that's probably less Pythonic

@dsmmcken
Copy link
Contributor

what about just context_menu and context_header_menu?

@mattrunyon mattrunyon requested a review from mofojed June 13, 2024 21:23
@mattrunyon
Copy link
Collaborator Author

Might be able to add async menu items to this PR if the callables returning callables PR merges first. Feels like that one is close

plugins/ui/src/js/src/elements/UITable.tsx Outdated Show resolved Hide resolved
plugins/ui/src/js/src/elements/UITable.tsx Outdated Show resolved Hide resolved
plugins/ui/docs/README.md Outdated Show resolved Hide resolved
plugins/ui/src/js/src/elements/UITable.tsx Outdated Show resolved Hide resolved
@mattrunyon mattrunyon requested a review from mofojed June 24, 2024 14:38
mattrunyon added a commit to deephaven/web-client-ui that referenced this pull request Jun 24, 2024
Copy link
Member

@mofojed mofojed left a comment

Choose a reason for hiding this comment

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

Used a whole bunch of menus for testing:

from deephaven import empty_table, ui
import time

t = empty_table(100).update(["x=i", "y=Math.sin(x)"])


def action_context_menu(e):
    return [
        {
            "title": f"Action for {e['value']}",
            "action": lambda d: print(d),
        }
    ]


def delayed_context_menu(e):
    time.sleep(2)
    return [
        {
            "title": f"Delayed action for {e['value']}",
            "action": lambda d: print(d),
        }
    ]


def sub_menus(e):
    return [
        {
            "title": f"Submenu for {e['value']}",
            "actions": [action_context_menu, delayed_context_menu],
        }
    ]


def delayed_sub_menus(e):
    time.sleep(2)
    return [
        {
            "title": f"Delayed submenu for {e['value']}",
            "actions": [action_context_menu, delayed_context_menu],
        }
    ]


def even_menu_only(e):
    if e["value"] % 2 == 0:
        return [
            {
                "icon": 'vsWarning',
                "title": f"Even menu for {e['value']}",
                "action": lambda d: print(d),
                "order": 1,
            }
        ]


action_t = ui.table(t, context_menu=[action_context_menu])

delayed_t = ui.table(
    t,
    context_menu=[delayed_context_menu],
)

all_t = ui.table(
    t,
    context_menu=[
        even_menu_only,
        action_context_menu,
        delayed_context_menu,
        sub_menus,
        delayed_sub_menus,
    ],
)

Looks like it works well so far. Great work!

plugins/ui/DESIGN.md Show resolved Hide resolved
@@ -23,6 +24,8 @@ def table(
quick_filters: dict[ColumnName, QuickFilterExpression] | None = None,
show_quick_filters: bool = False,
show_search: bool = False,
context_menu: list[ResolvableContextMenuItem] | None = None,
Copy link
Member

Choose a reason for hiding this comment

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

So in web-client-ui, we accept an array or just a single item: https://github.com/deephaven/web-client-ui/blob/e13c9d78decdfba2ff76657a024b2df44f2ae0fc/packages/components/src/context-actions/ContextActionUtils.ts#L5

Would be nice if we could handle that. Right now doing this is throwing an error on the JS side on right-click and isn't reported to the user, we should at least handle that better:

from deephaven import empty_table, ui

t = ui.table(
    empty_table(100).update(["x=i", "y=Math.sin(x)"]),
    context_menu=lambda e: {
        "title": f"Action for {e['value']}",
        "action": lambda d: print(d),
    },
)

Currently you need to wrap both the function and the return value from the function in arrays, and you don't need to do either in the ContextAction in web-client-ui:

from deephaven import empty_table, ui

action_t = ui.table(
    empty_table(100).update(["x=i", "y=Math.sin(x)"]),
    context_menu=[
        lambda e: [
            {
                "title": f"Action for {e['value']}",
                "action": lambda d: print(d),
            }
        ]
    ],
)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ya I was mostly trying to make the types a bit simpler for Python. I can make it support both though

plugins/ui/src/deephaven/ui/types/types.py Show resolved Hide resolved
plugins/ui/src/js/src/elements/utils/UITableUtils.tsx Outdated Show resolved Hide resolved
});
}
: undefined,
actions: item.actions ? wrapContextActions(item.actions, data) : undefined,
Copy link
Member

Choose a reason for hiding this comment

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

We should also allow order to be passed in here... right now it's implicitly sorting based on the title, which isn't the best.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Why do you say it is sorting by title? It should just be sorted by the order the actions are defined in Python.

We should allow order, but I figured that could be a followup. I don't really like just saying "well our current menu items are 500, 800, 1000" or whatever they are

Also I think you could pass an order right now since the item gets spread before setting the action/actions

@mattrunyon mattrunyon requested a review from mofojed June 27, 2024 22:40
mofojed pushed a commit to deephaven/web-client-ui that referenced this pull request Jun 28, 2024
…o be more permissive (#2117)

Needed for proper TS in
deephaven/deephaven-plugins#522. Otherwise the
class complains it can't properly extend `IrisGridContextMenuHandler`.
@mattrunyon mattrunyon merged commit 32d09e8 into deephaven:main Jun 28, 2024
14 checks passed
@mattrunyon mattrunyon deleted the table-context-menu branch June 28, 2024 21:26
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.

ui.table Context menu actions (.context_menu)
3 participants