Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

feat!: add block definition to multiline text field plugin #2202

Merged
merged 5 commits into from
Feb 23, 2024

Conversation

rachel-fenichel
Copy link
Collaborator

@rachel-fenichel rachel-fenichel commented Feb 15, 2024

The basics

The details

Resolves

Fixes #2154

Proposed Changes

  • Adds a block definition and generators for one block associated with the multiline input field:
    • text_multiline
  • Follows the previously established pattern for defining and exporting a library of blocks

BREAKING CHANGE: The multiline text input field no longer registers itself on load. The developer must either manually register the field or install blocks, which will install the field. This is part of a move to have no side effects in field and block definitions, so that tree-shaking can remove unwanted fields and blocks.

Reason for Changes

This is the next step in extracting block definitions from the core library and publishing them as smaller libraries. Developers can then choose which blocks and fields to install, and can tree-shake out unneeded ones.

Test Coverage

  • Updated mocha tests to call registerFieldMultilineInput
  • Updated test playground to delete/unregister the old block, generators, and field from core, then install/register the new versions.
  • Stripped use of old generateFieldTestBlocks helper in favor of manually defining test blocks and the test toolbox.
    • I found the result easier to read and modify, and it won't change frequently.

The test toolbox looks like this:
image

The first part of the toolbox tests the exported blocks. The later sections test the implementation of the field.

Documentation

Updated README to describe the block and how to use it, and to note that users must install either the block or the field.

Additional Information

Open question: this field only exports one block. I still also exported installAllBlocks. The only value there is consistency. Should I even bother for a single block? In the example code and README I call textMultiline.installBlock().

As with google/blockly#2162, there is some follow-up work:

  • Port generator tests from core, to test generated code against golden files.
    • For the purposes of this PR I tested by loading the block in the playground and checking the generated code in the playground.
  • Other cleanup tracked in Cleanup after deleting blocks and fields from core #2194

@rachel-fenichel rachel-fenichel changed the title Multiline feat!: add block definition to multiline text field plugin Feb 15, 2024
@rachel-fenichel rachel-fenichel marked this pull request as ready for review February 15, 2024 20:19
@rachel-fenichel rachel-fenichel requested a review from a team as a code owner February 15, 2024 20:19
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.

Open question: this field only exports one block. I still also exported installAllBlocks. The only value there is consistency. Should I even bother for a single block? In the example code and README I call textMultiline.installBlock().

I think it's worth providing an installAllBlocks for consistency and in case we add more blocks in future—but it can just be an alias for the oen installBlock function for now.

Reciprocal question: I presume you have not attempted to address google/blockly#7209, correct? I ask because the toolbox screenshot seems to be rendered better than I'd expect given that bug.

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.

(Duplicate comment body deleted.)


// Installs the block, the field, and all of the language generators.
textMultiline.installBlock({
javascript: javascriptGenerator,
Copy link
Contributor

Choose a reason for hiding this comment

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

Might it be worth adding an "all items optional" comment here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The text above says "you can supply one or more CodeGenerator instances", which i think is sufficient.

Comment on lines +194 to +205
/**
* Uninstall the base multiline text block and its associated generators.
* TODO(#2194): remove this when those blocks are removed from the core library.
*/
function uninstallBlock() {
delete Blockly.Blocks['text_multiline'];
delete javascriptGenerator.forBlock['text_multiline'];
delete dartGenerator.forBlock['text_multiline'];
delete luaGenerator.forBlock['text_multiline'];
delete pythonGenerator.forBlock['text_multiline'];
delete phpGenerator.forBlock['text_multiline'];
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Why wrap this in a function that is called from createWorkspace, rather than just executing the contents directly a the top level any time before calling document.addEventListener?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I find it easy to lose track of where things that are specific to the plugin test are, so I like putting it all into createWorkspace. This is a slight personal preference.

@rachel-fenichel
Copy link
Collaborator Author

As discussed offline, Blake has a fix for the multiline field issue in #2208

@rachel-fenichel rachel-fenichel merged commit c29c022 into google:blocks_for_fields Feb 23, 2024
7 of 8 checks passed
@rachel-fenichel rachel-fenichel deleted the multiline branch February 23, 2024 19:41
rachel-fenichel added a commit that referenced this pull request Mar 29, 2024
* chore: force-install blockly 10.4.0-beta.1 for development

* feat: add text_multiline block and associated generators

* feat: update test page to use new block and field

* feat: README

* chore: respond to PR feedback

BREAKING CHANGE: The multiline text input field no longer registers itself on load. The developer must either manually register the field or install blocks, which will install the field. This is part of a move to have no side effects in field and block definitions, so that tree-shaking can remove unwanted fields and blocks.
rachel-fenichel added a commit that referenced this pull request Apr 2, 2024
* chore: force-install blockly 10.4.0-beta.1 for development

* feat: add text_multiline block and associated generators

* feat: update test page to use new block and field

* feat: README

* chore: respond to PR feedback

BREAKING CHANGE: The multiline text input field no longer registers itself on load. The developer must either manually register the field or install blocks, which will install the field. This is part of a move to have no side effects in field and block definitions, so that tree-shaking can remove unwanted fields and blocks.
rachel-fenichel added a commit that referenced this pull request Apr 2, 2024
* chore: force-install blockly 10.4.0-beta.1 for development

* feat: add text_multiline block and associated generators

* feat: update test page to use new block and field

* feat: README

* chore: respond to PR feedback

BREAKING CHANGE: The multiline text input field no longer registers itself on load. The developer must either manually register the field or install blocks, which will install the field. This is part of a move to have no side effects in field and block definitions, so that tree-shaking can remove unwanted fields and blocks.
maribethb pushed a commit that referenced this pull request Apr 2, 2024
* feat!: add block definitions to colour field plugin (#2162)

* feat: export functions to register some angle, colour, and multiline input fields

* feat: add block definitions to colour and multiline input fields

* feat: add block generators for the colour_picker block

* fix: use Blockly.common.defineBlocksWithJsonArray

* feat: add block generators for colour_random block

* feat: add generators type and standardize exports for block files

* chore: update to blockly@beta and fix types

* chore: move all colour blocks to separate files and add more generator-related types

* feat: finish adding block code generators for colour blocks

* fix: PR feedback

* fix: remove immediate registration of blocks and fields in field_colour

* chore: use named imports and numbered TODOs

* feat: add usage information about blocks to README

* chore: revert changes outside of field_colour

* chore: clean up tsdoc and exports

* cgire: clean up README

* chore: respond to PR feedback on names and comments

* feat(tests): add and improve tests

* chore(format): run formatter

* fix: respond to PR feedback about names and file structure

* fix: allow const variables to have UPPER_CASE names

* fix: respond to PR feedback

* chore: format

* fix: line length

* feat!: add block definition to multiline text field plugin (#2202)

* chore: force-install blockly 10.4.0-beta.1 for development

* feat: add text_multiline block and associated generators

* feat: update test page to use new block and field

* feat: README

* chore: respond to PR feedback

BREAKING CHANGE: The multiline text input field no longer registers itself on load. The developer must either manually register the field or install blocks, which will install the field. This is part of a move to have no side effects in field and block definitions, so that tree-shaking can remove unwanted fields and blocks.

* chore: format (#2221)

* feat!: Add registration function to field_angle and make it no longer install on file load (#2211)

* feat!: Add registration function to field_angle and make it no longer install on file load

* chore: formatting

BREAKING CHANGE: The angle field no longer registers itself on load. The developer must manually register the field. This is part of a move to have no side effects in field and block definitions, so that tree-shaking can remove unwanted fields and blocks.

* feat: add tests for per-block generators in field-colour (#2220)

* feat: add unit tests for generators

* fix: imports in tests

* chore: format

* feat: ignore golden files in plugin tests

* chore: fix lint

* feat: ignore golden files for linting

* fix: revert addition of fs and path packages

* fix: remove suite.only to make all tests run

* chore: handle review feedback

* fix: only import what you need in colour blocks

* feat: add generator tests for the text_multiline block (#2232)

* feat: add generator tests for the text_multiline block

* fix: updated test string to include multiple types of quotes

* fix: code style in generators for field colour blocks (#2233)

* feat!: update blockly version to 10.4.3 for colour and multilineinput (#2296)

* feat!: update blockly version to 10.4.3 for colour and multilineinput

* chore!: update peer dependencies

* chore: update package-lock.jsons

* chore: fix dependencies
gonfunko pushed a commit to gonfunko/blockly-samples that referenced this pull request Apr 15, 2024
* feat!: add block definitions to colour field plugin (google#2162)

* feat: export functions to register some angle, colour, and multiline input fields

* feat: add block definitions to colour and multiline input fields

* feat: add block generators for the colour_picker block

* fix: use Blockly.common.defineBlocksWithJsonArray

* feat: add block generators for colour_random block

* feat: add generators type and standardize exports for block files

* chore: update to blockly@beta and fix types

* chore: move all colour blocks to separate files and add more generator-related types

* feat: finish adding block code generators for colour blocks

* fix: PR feedback

* fix: remove immediate registration of blocks and fields in field_colour

* chore: use named imports and numbered TODOs

* feat: add usage information about blocks to README

* chore: revert changes outside of field_colour

* chore: clean up tsdoc and exports

* cgire: clean up README

* chore: respond to PR feedback on names and comments

* feat(tests): add and improve tests

* chore(format): run formatter

* fix: respond to PR feedback about names and file structure

* fix: allow const variables to have UPPER_CASE names

* fix: respond to PR feedback

* chore: format

* fix: line length

* feat!: add block definition to multiline text field plugin (google#2202)

* chore: force-install blockly 10.4.0-beta.1 for development

* feat: add text_multiline block and associated generators

* feat: update test page to use new block and field

* feat: README

* chore: respond to PR feedback

BREAKING CHANGE: The multiline text input field no longer registers itself on load. The developer must either manually register the field or install blocks, which will install the field. This is part of a move to have no side effects in field and block definitions, so that tree-shaking can remove unwanted fields and blocks.

* chore: format (google#2221)

* feat!: Add registration function to field_angle and make it no longer install on file load (google#2211)

* feat!: Add registration function to field_angle and make it no longer install on file load

* chore: formatting

BREAKING CHANGE: The angle field no longer registers itself on load. The developer must manually register the field. This is part of a move to have no side effects in field and block definitions, so that tree-shaking can remove unwanted fields and blocks.

* feat: add tests for per-block generators in field-colour (google#2220)

* feat: add unit tests for generators

* fix: imports in tests

* chore: format

* feat: ignore golden files in plugin tests

* chore: fix lint

* feat: ignore golden files for linting

* fix: revert addition of fs and path packages

* fix: remove suite.only to make all tests run

* chore: handle review feedback

* fix: only import what you need in colour blocks

* feat: add generator tests for the text_multiline block (google#2232)

* feat: add generator tests for the text_multiline block

* fix: updated test string to include multiple types of quotes

* fix: code style in generators for field colour blocks (google#2233)

* feat!: update blockly version to 10.4.3 for colour and multilineinput (google#2296)

* feat!: update blockly version to 10.4.3 for colour and multilineinput

* chore!: update peer dependencies

* chore: update package-lock.jsons

* chore: fix dependencies
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants