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

fix: custom block context menus #5976

Merged
merged 2 commits into from
Mar 3, 2022

Conversation

BeksOmega
Copy link
Collaborator

The basics

  • I branched from develop
  • My pull request is against develop
  • My code follows the style guide

The details

Resolves

#5975

Proposed Changes

Moves the context menu property definition to the block instead of block svg.

Behavior Before Change

No custom context menus =(

Behavior After Change

Custom context menus!

Reason for Changes

Any custom context menu function that was getting mixed into the block was getting overwritten by the block svg constructor. This fixes that.

Test Coverage

Manually tested that the procedure context menus appear.

Tested on:

  • Desktop Chrome

Documentation

N/A

Additional Information

Thanks for the catch Christopher!

@BeksOmega BeksOmega requested a review from a team as a code owner March 3, 2022 15:54
@BeksOmega BeksOmega requested review from cpcallen and removed request for rachel-fenichel March 3, 2022 17:09
@BeksOmega BeksOmega assigned cpcallen and unassigned rachel-fenichel Mar 3, 2022
@BeksOmega
Copy link
Collaborator Author

Per offline discussion, I've moved the properties back into BlockSvg (since they are rendered-specific) and changed the assignment to be identity (eg this.decompose = this.decompose) to avoid overwriting mixed-in properties.

Copy link
Contributor

@cpcallen cpcallen left a comment

Choose a reason for hiding this comment

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

As noted in our conversation:

  • It's best to actually initialise any properties defined in a class constructor, to keep the shapes of the resulting objects consistent, but where a property is being redefined (to change its type) it is fine to just mention it without initialisation.
  • It would be better not to move properties used only by BlockSvg to Block.
  • Where a property in the subclass constructor is initialised to undefined, an initialisation like this.prop = this.prop will ensure it exists and is set to undefined if it was not set by the mixin.
  • Changing the type of a superclass property in a subclass and user-defined mixins are both… problematic, as far as type-checking goes—but that is a problem for another day quarter.

tests/deps.js Show resolved Hide 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.

3 participants