refactor: convert toolbox items to ES6 classes #5947
Conversation
|
@maribethb Just wanted to give you a heads up, I requested your review because you're the most recent person to use this API, and I wanted to make sure I didn't mess things up for external peeps subclassing the categories. |
There was a problem hiding this comment.
Your PR title should say refactor instead of refact. I feel like the conventional commit linter should have caught this... I don't know why it didn't since refact is not in the approved list https://www.npmjs.com/package/@commitlint/config-conventional but ¯\_(ツ)_/¯
| @@ -253,7 +253,6 @@ | |||
| "./core/extensions.js", | |||
| "./core/block.js", | |||
| "./core/utils/string.js", | |||
| "./core/utils/object.js", | |||
There was a problem hiding this comment.
what's up with this change? is this the last file that is using object.inherits?
There was a problem hiding this comment.
No clue, something removed it automatically. But if you expand this code block down, it looks like it still exists link. So I don't think I need to add it back in?
|
Should be ready for another look! |
There was a problem hiding this comment.
Looks good although there were a few comments from round 1 that weren't addressed yet (make sure you update the PR title so that this gets categorized correctly in the release notes please!)
Also would you mind updating the PR description to reflect the latest changes? If we come back to look at this PR it could be confusing since we decided not to use the sentinel value.
Thanks!
BeksOmega
changed the title
|
Ok @maribethb :D PR title and description are updated, |
The conventional commit linter only checks the shape of the commit message, unfortunately--it doesn't check against an approved list of types. |
The basics
The details
Resolves
Work on #5860
Proposed Changes
Converts toolbox item, category, and collapsible category to ES6 classes.
This involves adding a SENTINEL value very similar to the value used in fields so that we can make sure the toolbox category contents aren't parsed until all of the properties of the category instance have been defined.This involves moving the parsing of category contents into theinitmethod to ensure that all properties of the category instance have been defined before we try to modify them.Should be no change in behavior fingers crossed.
Reason for Changes
Moving to ES6 classes.
Test Coverage
All unit tests pass.
Documentation
N/A
Additional Information
N/A