Extract partitionBlocksByType into dedicated util function#53408
Conversation
| // Google Blockly only. Registers custom blocks for modal function editor. | ||
| }, | ||
| partitionBlocksByType() { | ||
| // TODO: Add comment |
There was a problem hiding this comment.
@molly-moen What do you think is the most useful way to phrase this comment?
There was a problem hiding this comment.
I'd probably do something like the comment on registerCustomProcedureBlocks that this function only applies to google blockly.
molly-moen
left a comment
There was a problem hiding this comment.
couple nits on naming, but I think this looks good!
| * @returns {Element[]|Object[]} A new array of block elements or JSON blocks partitioned based on their types. | ||
| */ | ||
| export function partitionBlocksByType(blockElements, prioritizedBlockTypes) { | ||
| export function partitionBlocksByType(blocks = [], options = {}) { |
There was a problem hiding this comment.
nit: is there a reason you went with an options object rather than having separate parameters for each option? I think separate parameters is more readable.
There was a problem hiding this comment.
I can switch back to separate parameters. In general, I think it makes the calls more readable to indicate what the flag means as part of the call to the function. That being said, happy to switch back/no strong feelings here!
| */ | ||
| export function partitionBlocksByType(blockElements, prioritizedBlockTypes) { | ||
| export function partitionBlocksByType(blocks = [], options = {}) { | ||
| const {prioritizedBlockTypes = [], isJson = false} = options; |
There was a problem hiding this comment.
nit: I wonder if there's a better name than isJson. In my mind the opposite of json is xml, which is not the false case here. Would it be better to use something like isBlockElements?
There was a problem hiding this comment.
isBlockElements works for me!
Before this PR, we had two versions of the
partitionBlocksByTypefunction, one incdoBlockSerializer.jsthat operated on JSON blocks usingblock.typeand one incdoXml.jsthat operated on block elements usingblock.getAttribute('type'). This PR creates a single version incdoUtils.jsthat can operate on either block elements or JSON blocks. Use ofblock.typeorblock.getAttribute('type')is controlled by the boolean argumentisBlockElements. I also added some additional tests.Links
https://codedotorg.atlassian.net/browse/SL-1088
Testing story
Added tests and included the test cases from the original implementation to confirm that they still pass.
Follow-up work
Rebase #53154.
PR Checklist: