Chart: custom context menu #4005

cnrudd · 2025-05-28T19:54:23Z

This addresses the following tickets:

Yes, you get 4 for 1 by merging this PR. :)

Corresponding Toolbox PR with new custom context menu on Line Chart: xh/toolbox#765

Hoist P/R Checklist

Pull request authors: Review and check off the below. Items that do not apply can also be
checked off to indicate they have been considered. If unclear if a step is relevant, please leave
unchecked and note in comments.

Caught up with develop branch as of last change.
Added CHANGELOG entry, or determined not required.
Reviewed for breaking changes: minor breaking change, noted in changelog.
Updated doc comments / prop-types, or determined not required.
Reviewed and tested on Mobile: not required. no context menu on mobile
Created Toolbox branch: Customized chart context menu on lineChart. toolbox#765

If your change is still a WIP, please use the "Create draft pull request" option in the split
button below to indicate it is not ready yet for a final review.

Pull request reviewers: when merging this P/R, please consider using a squash commit to
collapse multiple intermediate commits into a single commit representing the overall feature
change. This helps keep the commit log clean and easy to scan across releases. PRs containing a
single commit should be rebased when possible.

cnrudd · 2025-05-28T22:20:19Z

Still in progress.
I realized I could improve this by passing the series.point and original right click event to the custom actionFn, but have run into a low level issue where it looks like the rendered contextMenu component is cached. So, I am only getting the first context menu generated when clicking on a point for the first time. Subsequent clicks on other points still carry the first point clicked in the context menu.

cnrudd · 2025-05-29T15:31:06Z

Still in progress. I realized I could improve this by passing the series.point and original right click event to the custom actionFn, but have run into a low level issue where it looks like the rendered contextMenu component is cached. So, I am only getting the first context menu generated when clicking on a point for the first time. Subsequent clicks on other points still carry the first point clicked in the context menu.

Now ready for review, with point and rightClick event passed to actionFn

# Conflicts: # CHANGELOG.md

amcclain · 2025-06-02T19:57:23Z

Thanks for this @cnrudd - I will review this very shortly and reach out with any questions

# Conflicts: # CHANGELOG.md

amcclain

This was pre-existing code, but since we're looking at the context menu here, in Chart.ts render function we have

return !XH.isMobileApp ? useContextMenu(coreContents, impl.contextMenu) : coreContents;

Seems like we should check impl.contextMenu instead and return coreContents directly if null. That way the "not supported on mobile" fact is expressed once, and we also aren't wrapping the component in the case where the user has disabled the context menu.

Thanks for making this change - I think we have all the parts we need, just wondering in my review comments here about the "packaging" - specifically the roles of ChartModel vs. ChartLocalModel and the need for the new ChartContextMenu class vs a functional implementation. Have a look and LMK when you think when you have a chance.

cmp/chart/Chart.ts

amcclain · 2025-06-02T20:44:03Z

cmp/chart/Chart.ts

+                contextMenuClickEvt: e,
+                point
+            }).items;
+        };


Would it be more clear to have this work of determining the point and creating the new object done in the main ChartModel, with the render function getting it directly off of the main model vs this impl?

What's ChartLocalModel doing for us here?

cmp/chart/ChartModel.ts

desktop/cmp/contextmenu/ChartContextMenu.ts

cnrudd · 2025-06-03T23:18:54Z

@amcclain @lbwexler This is ready for a 2nd look.

I've implemented these changes:

reworked as @amcclain suggested so that menu item building is no longer in a class (and implemented all of the smaller suggestions)
added contextMenuEvent to standard context menu (Lee's suggestion)
chart actionFn: grouped args in 'params' arg (Lee's suggestion)
chart actionFn: support providing points (for when more than one series is in chart and tooltip is shared)
also fixed chart mouse right to left zoom out bug
added/improved toolbox demos: multi series chart, new context menu item on basicPanel demo

amcclain · 2025-06-04T13:19:16Z

cmp/chart/impl/ChartContextMenuItems.ts

+
+/**
+ * If a String, value can be '-' for a separator, or a token supported by HighCharts
+ * for its native menu items, or a Hoist specific token.


Can it also be any arbitrary string to insert a header? If not, think we should spec '-' vs string here

amcclain · 2025-06-04T13:20:52Z

cmp/chart/impl/ChartContextMenuItems.ts

+                actionFn: () => chartModel.highchart.downloadXLS()
+            };
+        default:
+            return token;


Same question here - wondering if this can only be - or...

amcclain · 2025-06-04T13:21:58Z

cmp/chart/impl/ChartContextMenuItems.ts

+            return {
+                text: 'View in full screen',
+                icon: Icon.expand(),
+                hidden: !chartModel,


I notice that all of these have this hidden: !chartModel - are we ever rendering this menu without a chartModel? Should there be some early out before we reach this point?

amcclain · 2025-06-04T13:24:22Z

cmp/chart/impl/ChartContextMenuItems.ts

+    | 'downloadPDF'
+    | 'copyToClipboard';
+
+export interface ChartMenuItem extends Omit<MenuItem, 'actionFn' | 'items'> {


Worth making the naming consistent - we could drop Context from the other exported members here (there's no other kind of menu), or make this ChartContextMenuItem

amcclain · 2025-06-04T13:28:36Z

cmp/chart/impl/ChartContextMenuItems.ts

+ * If a String, value can be '-' for a separator, or a token supported by HighCharts
+ * for its native menu items, or a Hoist specific token.
+ */
+export type ChartContextMenuItemLike = ChartMenuItem | ChartContextMenuToken | string;


Sorry I'm still a little confused by ChartContextMenuItemLike and ChartMenuItemLike below - just having a hard time following all the variations in this file, not sure if it's the naming, or if we have some extra layer or something..... Left wondering if any way to streamline a bit. Maybe if we clarify the role of the '-' string it would help.

amcclain · 2025-06-04T13:29:39Z

cmp/chart/impl/ChartContextMenuItems.ts

+}
+
+function isMenuItem(item: ChartMenuItemLike): item is MenuItem {
+    return !isString(item) && !isValidElement(item);


This is called in just one place here, and we've already checked if the value is a string and won't reach here if so - could inline the validElement check and drop this function which I think just adds a bit more noise to the file at the moment

amcclain · 2025-06-04T13:30:45Z

cmp/chart/impl/zoomout.ts

@@ -30,7 +30,8 @@ export function installZoomoutGesture(Highcharts) {

        Highcharts.addEvent(this, 'selection', e => {
            if (pixelDiff < 0) {
-                this.zoom(); // call w/o arguments resets zoom
+                // `undefined`'s preserve defaults, last arg `false` disables animation


What's this change doing? Doesn't seem related to the others, or is it?

amcclain · 2025-06-04T13:34:52Z

core/types/Interfaces.ts

@@ -284,10 +284,10 @@ export interface MenuItem {
    className?: string;

    /** Executed when the user clicks the menu item. */
-    actionFn?: (e: MouseEvent | PointerEvent) => void;
+    actionFn?: (e: MouseEvent | PointerEvent, contextMenuEvent?: MouseEvent | PointerEvent) => void;



So the first event e is the click on a specific item within the menu, and the new second event contextMenuEvent is the right-click that caused the menu to appear in the first place?

amcclain · 2025-06-04T13:35:58Z

desktop/cmp/contextmenu/ContextMenu.ts

@@ -20,6 +20,12 @@ export type ContextMenuSpec = MenuItemLike[] | ((e: MouseEvent) => MenuItemLike[

 export interface ContextMenuProps extends HoistProps {
    menuItems: MenuItemLike[];
+    /**
+     * Optional event because this component is sometimes used as the content in popovers, to
+     * create menu buttons.  In those cases, it is not expected that the click event contains any


because this component is sometimes used as the content in popovers, to create menu buttons

Is this not always how it's used? What's an example of the "contextual information" here, or when would you expect to find something in the first event e? Just as a developer I'm left a little uncertain as to what I would do with one event or the other.

amcclain · 2025-06-04T13:38:54Z

cmp/chart/impl/ChartContextMenuItems.ts

+    | ChartContextMenuItemLike[]
+    | ((chartModel: ChartModel) => ChartContextMenuItemLike[]);
+
+export function getChartContextMenuItems(


Should we mark as @internal?

amcclain · 2025-06-04T13:39:32Z

cmp/chart/ChartModel.ts

+        if (isNil(spec) || spec === true) return ChartModel.defaultContextMenu;
+
+        return spec;
+    }


This looks great / very clear

amcclain · 2025-06-04T13:40:23Z

cmp/chart/ChartModel.ts

-    /** True to showContextMenu.  Defaults to true.  Desktop only. */
-    showContextMenu?: boolean;
+    /**
+     * True to show default ContextMenu.  Defaults to true.  Desktop only.


Suggested change

* True to show default ContextMenu. Defaults to true. Desktop only.

* True (default) to show default ContextMenu. Supported on desktop only.

amcclain · 2025-06-04T13:47:46Z

cmp/chart/Chart.ts

@@ -106,7 +108,7 @@ class ChartLocalModel extends HoistModel {
    model: ChartModel;

    chartRef = createObservableRef<HTMLElement>();
-    contextMenu: any;
+    contextMenu: (e) => MenuItemLike[];


Sorry we can leave this as is, but still feels like there's some way to streamline a bit more.

Currently we have

ChartLocalModel.contextMenu - function created/captured as soon as linked

ChartLocalModel.getContextMenu() - function to create the above function

ChartModel.contextMenu - partially processed/parsed spec, might be a function that this local model calls, passing back reference to ChartModel itself

getChartContextMenuItems() exported util

Still wondering what we're getting from having the context menu stuff in this local model - I am probably missing some subtlety as to why we need this layer involved, but whatever that subtlety is, if we could make more clear might help whenever we revisit this code in the future.

Or maybe we could drop contextMenu from this local model altogether and have the primary ChartModel process the specs/tokens it's given "all the way" into the function that ChartLocalModel.getContextMenu() currently produces.

So we'd have context menu stuff in two places - the main ChartModel and the helper util - and ChartModel in its ctor would process the specs it's passed all the way through to the form that the rendering within Chart.ts consumes and passes on to the useContextMenu hook.

Don't mean to be a pain with all of this, but am having a hard time keeping track of the current flow and if it could be simpler, great - if not, no worries :)

Tight up ChartContextMenuItemLike spec.

cnrudd added 3 commits May 28, 2025 15:47

Customizable chart contextMenu implemented: breaking change

34a67be

TS and Docs cleanups. Refactoring.

7c99514

Docs cleanup.

bb7cd5d

cnrudd mentioned this pull request May 28, 2025

Customized chart context menu on lineChart. xh/toolbox#765

Open

cnrudd requested review from lbwexler, febbraiod and amcclain May 28, 2025 19:55

TS fix

3a1e997

cnrudd marked this pull request as draft May 28, 2025 22:17

cnrudd added 2 commits May 29, 2025 10:59

Expose clicked chart point to menu item actionFn

804ed86

TS cleanups

0752131

cnrudd marked this pull request as ready for review May 29, 2025 15:25

cnrudd added 2 commits May 29, 2025 12:07

Revert unneeded change.

16bf2ce

Merge branch 'develop' into chart-custom-context-menu

3b83e4e

# Conflicts: # CHANGELOG.md

Merge branch 'develop' into chart-custom-context-menu

ae20c48

# Conflicts: # CHANGELOG.md

amcclain reviewed Jun 2, 2025

View reviewed changes

cnrudd added 7 commits June 3, 2025 11:40

Minor cleanups from CR with Lee and Anselm

2b0bb0b

Rework actionFn to take params object as arg

707568f

Convert class to methods

aaf99ac

Fix zoomout gesture. API must have changed.

084311d

Turn off animation on zoomOut

107bdf7

Support points array for multiple series with tooltip.shared = true.

a9fc1eb

Support passing contextMenuEvent to contextMenu prepareFn and actionFn

bd9d79f

amcclain reviewed Jun 4, 2025

View reviewed changes

cnrudd added 9 commits June 4, 2025 10:03

CR from Anselm: Remove unneeded hidden key in chart token parser.

23a93db

Tight up ChartContextMenuItemLike spec.

Merge branch 'develop' into chart-custom-context-menu

3f6dfe6

Merge branch 'develop' into chart-custom-context-menu

b170c32

pass contextMenuEvent where possible.

25743ea

Doc edit.

e90f00e

mark getChartContextMenuItems as @internal

7eb5e94

dropped Context from ChartContextMenuToken

fa80e9d

inline isValidElement check

23bae4d

Eliminate need for ChartMenuItemLike by threading '-' through

f68d08b

	* True to show default ContextMenu. Defaults to true. Desktop only.
	* True (default) to show default ContextMenu. Supported on desktop only.

Chart: custom context menu #4005

Are you sure you want to change the base?

Chart: custom context menu #4005

Uh oh!

Conversation

cnrudd commented May 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Hoist P/R Checklist

Uh oh!

cnrudd commented May 28, 2025

Uh oh!

cnrudd commented May 29, 2025

Uh oh!

amcclain commented Jun 2, 2025

Uh oh!

amcclain left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cnrudd commented Jun 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cnrudd commented May 28, 2025 •

edited

Loading

cnrudd commented Jun 3, 2025 •

edited

Loading