Skip to content

Conversation

@tashee
Copy link
Contributor

@tashee tashee commented Oct 22, 2025

The basics

The details

Resolves

Fixes issue #8786

Proposed Changes

Add getter to ContextMenu

Reason for Changes

Currently, the ContextMenu module provides no way to get at the underlying Menu instance, which may be convenient.

Test Coverage

Tests to be added separately.

Documentation

N/A

Additional Information

N/A

@tashee tashee requested a review from a team as a code owner October 22, 2025 12:08
@tashee tashee requested a review from BenHenning October 22, 2025 12:08
@tashee tashee requested review from cpcallen and gonfunko October 22, 2025 12:09
@BenHenning
Copy link
Contributor

Thanks @tashee! Apologies for the delayed response here. Some requests:

  • The lint & label workflow is failing. I think this is due to your PR title being incorrectly formatted. Can you please fix that to match other PRs? E.g. 'fix: ' is a great format to use.
  • Could you perhaps go ahead and add tests for the new getter in this PR? It's generally better to add tests with the code change so that we can help verify that it works and not forget to add them later. :)
  • Could you please sync with the latest develop changes as well? That should fix the welcome contributor workflow.

The keyboard navigation webdriver tests should be unrelated flakes from what I can tell.

Comment on lines +62 to +64
* Gets Menu instance.
*
* @returns menu instance.
Copy link
Contributor

Choose a reason for hiding this comment

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

One thing that would be really helpful to include is context on the menu's lifecycle. Specifically:

  • When is it defined? (Relative to other functions called here)
  • When is null returned? What does that mean/what can the caller infer from receiving a null value?
  • Is it valid to save the returned value or could that cause problems (e.g. if the menu is later closed)?

These would be great to clarify in the documentation here for a more solid defined API contract for callers.

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.

4 participants