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

Reorganize conditionals for block context menu #2390

Merged
merged 2 commits into from
Apr 18, 2019

Conversation

ahigerd
Copy link
Contributor

@ahigerd ahigerd commented Apr 18, 2019

The basics

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

The details

Resolves

Issue #2388 - Comments should be allowed on non-movable/deletable blocks

Proposed Changes

Regardless of deletable state:

  • Add the "Add Comment"/"Remove Comment" menu items whenever the block is editable and not collapsed.
  • Add the "Expand Block"/"Collapse Block" menu items whenever the block is movable.
  • Add the "Inline Inputs"/"External Inputs" menu items whenever the block is movable and not collapsed.
  • Add the "Disable Block"/"Enable Block" menu items whenever the block is editable.

Block Factory:

  • Turned off comments and block disabling in the Blockly.inject() call.

Reason for Changes

Previously, most of the context menu entries required that the block must be movable and deletable in addition to being editable. However, there's no compelling reason for these strict requirements: a block that can be edited should still be able to have a comment even if it can't be moved or deleted.

While making this change, I noticed that the other menu items could have their restrictions similarly loosened. Changing the visual organization of a block's inputs can be considered a "move" operation, so the inline/external toggle and the collapse/expand toggle don't need to be blocked by the deletable or editable flags. Similarly, changing the enabled/disabled state of a block can be considered an "edit" operation and it doesn't need to be blocked by the deletable or movable flags.

With the more permissive flags, existing code such as blockfactory have menu items appear that don't make sense in context. Since blockfactory is used for more than just a demo, I made sure to update the configuration.

Test Coverage

Playground testing:

  • Tested combinations of settings in personal project
  • Tested combinations of settings in blockfactory demo

Tested on:

  • Desktop Chrome
  • Desktop Firefox
  • Desktop Safari

Additional Information

@RoboErikG RoboErikG merged commit bc77024 into google:develop Apr 18, 2019
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.

None yet

2 participants