feat: break input types into separate classes #7019
Conversation
|
@maribethb I added you as a reviewer purely for the design-y / api-y bits of this. No need to do a nitty-gritty code review I (unless you want to), just want feedback on the approach. |
| @@ -71,8 +71,11 @@ import {Gesture} from './gesture.js'; | |||
| import {Grid} from './grid.js'; | |||
| import {Icon} from './icon.js'; | |||
| import {inject} from './inject.js'; | |||
| import {Align, Input} from './input.js'; | |||
| import {inputTypes} from './input_types.js'; | |||
| import {Align, Input} from './inputs/input.js'; | |||
There was a problem hiding this comment.
Is the intention for developers to use Blockly.inputs.Input now? Can you fix the comments on the @see tags for ALIGN_LEFT et al in this file (line 200 or so) if so?
Although, if we do want people to change the import path for input, now might be a good time to break the Align enum into its own file, so that we can eventually get rid of the Input namespace, which is not recommended and makes our documentation more confusing. We could continue to export the new enum from the old location for now.
So new would be
Blockly.input.Input and Blockly.input.Align.LEFT
and old (deprecated, to be removed) would be Blockly.Input and Blockly.Input.Align.LEFT aka Blockly.ALIGN_LEFT
wdyt? If you don't intend external developers to update their usage then this is all moot, I just think it might be nice to fix while we're here and changing things.
There was a problem hiding this comment.
I think that's reasonable! That would be a breaking change though correct? If so I think it would be better too do that in a separate PR so we can link to it & include just what broke.
There was a problem hiding this comment.
I think we should make it a deprecating change, and remove the old stuff in v11. But yes, doing it in its own PR so we can link to it from release notes makes sense.
ba4a450 to
d6489f4
Compare
The basics
npm run formatandnpm run lintThe details
Resolves
Work on #2062
Proposed Changes
Splits all of the different types of inputs out into individual classes.
Reason for Changes
This matches how we want external developers to define custom inputs. And also matches people's mental models better.
Test Coverage
N/A - Should be covered by existing tests.
Documentation
N/A
Additional Information
We could mark the constructors for the new inputs
@internalout of an abundance of caution. But I don't think there's any real risk to letting external developers construct them.Best to review commit-wise!