Skip to content

Comments

Convert the thumbnail context menu to display in React (BL-15895)#7702

Open
StephenMcConnel wants to merge 1 commit intomasterfrom
BL-15895-ReactPageThumbnailMenu
Open

Convert the thumbnail context menu to display in React (BL-15895)#7702
StephenMcConnel wants to merge 1 commit intomasterfrom
BL-15895-ReactPageThumbnailMenu

Conversation

@StephenMcConnel
Copy link
Contributor

@StephenMcConnel StephenMcConnel commented Feb 19, 2026


Open with Devin

This change is Reviewable

Copy link

@devin-ai-integration devin-ai-integration bot left a comment

Choose a reason for hiding this comment

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

✅ Devin Review: No Issues Found

Devin Review analyzed this PR and found no potential bugs to report.

View in Devin Review to see 5 additional findings.

Open in Devin Review

@hatton
Copy link
Member

hatton commented Feb 19, 2026

  1. JS requests menu items from C# → receives [{id, label, enabled}, ...]
  2. User clicks an item → JS sends the commandId back to C# for execution

@StephenMcConnel would it be more natural for the UI (typescript) to know the menu items it wants?

Copy link
Contributor Author

@StephenMcConnel StephenMcConnel left a comment

Choose a reason for hiding this comment

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

I thought about it but came up with two reasons for keeping the way the AI structured things:

  1. There would need to be an API call for each menu item to get its localized label.
  2. There would need to be an API call for each menu item to get its enabled state, although maybe that could be computed on the client side. I haven't really looked at that.
    Making a single API call to get all of the menu items with those values already computed seemed like a reasonable choice with
    simpler setup.

@StephenMcConnel made 1 comment.
Reviewable status: 0 of 4 files reviewed, all discussions resolved.

Copy link
Member

@hatton hatton left a comment

Choose a reason for hiding this comment

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

Yeah, let's "go native" for all of these menus, not rely on the server to act like part of the UI. We have UI all over in typescript land that manages to localize itself one way or the other :-)

@hatton made 1 comment.
Reviewable status: 0 of 4 files reviewed, all discussions resolved.

Copy link
Contributor Author

@StephenMcConnel StephenMcConnel left a comment

Choose a reason for hiding this comment

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

OK.

@StephenMcConnel made 1 comment.
Reviewable status: 0 of 4 files reviewed, all discussions resolved.

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