fix: overwriting flyout def with dynamic categories #6913
Conversation
There was a problem hiding this comment.
This looks fine, though it took me far too long to understand what was going on: my brain misparsed things and matched the } following return; at the end of the first if block in createItem as if it was the end of createItem, and I was deeply confused about why this helper function seemed to neither return a value nor have side effects.
I'm not sure if there's a good way to improve this code without breaking compatibility, but I think ideally there'd only be a single (top-level) function which recurses by calling itself.
Failing that, some comments noting that createFlyoutInfo_ is more or less just a nonrecursive wrapper for createInfo, and that the latter does all the heavy lifting, might make this easier for newcomers to understand.
| const categoryName = customInfo['custom']; | ||
| const flyoutDef = this.getDynamicCategoryContents_(categoryName); | ||
| const parsedDynamicContent = | ||
| toolbox.convertFlyoutDefToJsonArray(flyoutDef); | ||
| // Replace the element at i with the dynamic content it represents. | ||
| parsedContent.splice.apply( | ||
| parsedContent, [i, 1, ...parsedDynamicContent]); |
There was a problem hiding this comment.
Aaaargh! I'm glad this monstrosity is gone. I can understand how we got here, but who decided to keep the apply around once we got the spread operator??: parsedContent.splice(i, 1, ...parsedDynamicContent)!
The basics
npm run formatandnpm run lintThe details
Resolves
Fixes #4443
Proposed Changes
Stops mutating the toolbox definition when parsing dynamic categories.
Reason for Changes
See explanation in #4443
Test Coverage
Manual testing.
Can add unit testing after we confirm this is the behavior we actually want.
Documentation
N/A
Additional Information
Do we actually want flyout toolboxes to parse the dynamic categories? I kind of understand it in that you might want to embed variables / procedures in your flyout toolbox. But it also just seems.... weird?
Just want to confirm this before adding unit tests / merging.