Skip to content

#39401 make closeContextMenu optional #104

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.

Sign up for GitHub

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

Open
daniela-mateescu wants to merge 4 commits into
base: master-flower-platform
Choose a base branch
from
Open

#39401 make closeContextMenu optional #104

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8ee0a61
#39401 make `closeContextMenu` optional
daniela-buzatu Oct 24, 2025
e76bad4
#39401 Small refactor: remove unnecessary IParamsForAction
daniela-buzatu Oct 27, 2025
4a62b35
#39401 CHANGELOG.md
daniela-buzatu Oct 27, 2025
bfe7dd5
Merge remote-tracking branch 'origin/master-flower-platform' into
daniela-buzatu Apr 15, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
## Unreleased

* [Make `IActionParamForRun.closeContextMenu` optional](https://github.com/flower-platform/react-timeline-10000/pull/104)
* [Add `nowMarkerLiveUpdate` property that live updates the now marker](https://github.com/flower-platform/react-timeline-10000/pull/108)
* [Fix scrollbars that hide content on gantt/table](https://github.com/flower-platform/react-timeline-10000/pull/103)
* [Drag to create can start on top of an existing item](https://github.com/flower-platform/react-timeline-10000/pull/101)
Expand Down
12 changes: 2 additions & 10 deletions src/components/ContextMenu/ContextMenu.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,22 +1,14 @@
import React from 'react';
import { Menu, Popup, StrictPopupProps } from 'semantic-ui-react';
import { Menu, Popup } from 'semantic-ui-react';
import { IAction, IActionParamForRun } from './IAction';
import { TestsAreDemoCheat, createTestids } from '@famiprog-foundation/tests-are-demo';
import _ from 'lodash';

export type Point = { x: number, y: number };

type Position = StrictPopupProps["position"];

type IParamsForAction = {
selection: any[];
position?: Position;
[key: string]: any;
}
interface ContextMenuProps {
actions: IAction[];

paramsForAction: IParamsForAction;
paramsForAction: IActionParamForRun;
/**
* if undefined => the menu is closed else {x, y} position where the menu should open
*/
Expand Down
17 changes: 14 additions & 3 deletions src/components/ContextMenu/IAction.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,17 +1,24 @@
import React from "react"
import { IconProps, SemanticShorthandItem } from "semantic-ui-react"
import { IconProps, SemanticShorthandItem, StrictPopupProps } from "semantic-ui-react"
import { Point } from "./ContextMenu"

// TODO RM34271 let's get rid of IActionParamForRun. It overcomplicates the inheritance. We'll have here closeContextMenu() that won't do anything.
// export interface IActionParam<S> {
// selection: S[]
type Position = StrictPopupProps["position"];

export interface IActionParam {
selection: any[]
}

export interface IActionParamForRun extends IActionParam {
closeContextMenu: () => void,

/**
* Function to close the context menu.
* This is marked as optional because users will pass their actionParamForRun without this field.
* However, it will ALWAYS be populated internally by the ContextMenu component before being passed to action handlers.
*/
closeContextMenu?: () => void,

@gemini-code-assist gemini-code-assist Bot Oct 24, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

high

Making closeContextMenu optional is a breaking change for consumers of this library. Any action implementation that calls params.closeContextMenu() will now fail if the function is not provided. Previously, its presence was guaranteed by the type definition.

All call sites within consumer code will need to be updated to check for the existence of closeContextMenu before calling it, for example:

if (params.closeContextMenu) {
    params.closeContextMenu();
}

or using optional chaining:

params.closeContextMenu?.();

If this breaking change is intentional, it should be clearly documented in the pull request description and reflected in the package's versioning (e.g., a major version bump).

An alternative to making it optional, which would not be a breaking change, is to ensure it's always provided, even if it's a no-op function (() => {}) in contexts where there's no menu to close. This seems to align with the suggestion in the TODO comment on line 5 of this file.

@daniela-mateescu daniela-mateescu Oct 27, 2025

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

This is always populated internally in ContextMenu before passed to handlers. I have added a comment on top of this property


/**
* By default the context menu closes immediately after the action is run
* If the user wants to avoid the closing of the menu after action runs he needs to set this property to true
Expand All @@ -23,6 +30,10 @@ export interface IActionParamForRun extends IActionParam {
* It contains the mouse coordinates, obtained from onClick event of an action
*/
eventPoint?: Point

position?: Position;

[key: string]: any;
}

// TODO RM34271 CS: for the moment it's not clear why we need this. It's not used by the lib. And currently, the task of opening
Expand Down