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

Keyboard navigation should only use methods available on IFlyout #8063

Closed
1 task done
riknoll opened this issue May 2, 2024 · 3 comments · Fixed by #8064
Closed
1 task done

Keyboard navigation should only use methods available on IFlyout #8063

riknoll opened this issue May 2, 2024 · 3 comments · Fixed by #8064
Assignees
Labels
issue: feature request Describes a new feature and why it should be added

Comments

@riknoll
Copy link
Contributor

riknoll commented May 2, 2024

Check for duplicates

  • I have searched for similar issues before opening a new one.

Problem

In MakeCode, we implement a custom flyout that does not extend the base Flyout abstract class. In the keyboard navigation AST code, the flyout returned by workspace.getFlyout() is cast as a Flyout rather than an IFlyout, and uses methods that are implemented on that class but not included in the IFlyout interface.

For example, in this function, getContents() is part of Flyout and not IFlyout

Request

Keyboard navigation should use the IFlyout interface rather than the Flyout class to ensure compatibility with plugin contributed flyouts.

Alternatives considered

For now, I've just implemented the getContents() method on our Flyout.

Additional context

No response

@riknoll riknoll added issue: feature request Describes a new feature and why it should be added issue: triage Issues awaiting triage by a Blockly team member labels May 2, 2024
@maribethb maribethb self-assigned this May 3, 2024
@maribethb maribethb removed the issue: triage Issues awaiting triage by a Blockly team member label May 3, 2024
@maribethb
Copy link
Contributor

@riknoll fyi I think we have to keep this as a breaking change to require getContents in order to support accurate keyboard navigation. So I've formalized that in #8064 by adding it to the interface and removing the cast to Flyout.

Wanted to give you a heads up as I've changed the typing. I found your implementation and I think you'll just need to update your own typings of the function as well as remove the casts. Let me know if you anticipate problems with this. Thanks!

@gonfunko
Copy link
Contributor

gonfunko commented May 7, 2024

Without having looked at it closely, this seems perhaps related to google/blockly-samples#399. Just commenting to cross-link the two for future reference.

@riknoll
Copy link
Contributor Author

riknoll commented May 8, 2024

@maribethb should be no problem for us, thanks for the fix!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
issue: feature request Describes a new feature and why it should be added
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants