-
Notifications
You must be signed in to change notification settings - Fork 1.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Screen store TS conversion #15313
base: master
Are you sure you want to change the base?
Screen store TS conversion #15313
Conversation
QA Wolf here! As you write new code it's important that your test coverage is keeping up. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Few initial comments but looks good overall!
export interface ComponentState { | ||
components: Record<string, ComponentDefinition> | ||
customComponents: string[] | ||
selectedComponentId?: string | null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably use either an optional property or type it as string | null
shouldn't we? Rather than both. I've been opting to go for optional properties - the only change being we'll need to ensure we always clear these by setting them to undefined
rather than null
.
@@ -440,6 +442,11 @@ export class ComponentStore extends BudiStore<ComponentState> { | |||
* @returns | |||
*/ | |||
createInstance(componentName: string, presetProps: any, parent: any) { | |||
const screen = get(selectedScreen) | |||
if (!screen || !selectedScreen) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NAB: the !selectedScreen
part is redundant here.
interface ScreenState { | ||
screens: Screen[] | ||
selectedScreenId?: string | ||
selected?: Screen |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't see any external usages of this selected
property other than in the afterAction
of the history store constructor, so it'll always be null... I'm assuming this is maybe a leftover and we can refactor it out? I think in after action we should be reading the selectedScreen
derived store and using that to select a component instead of this selected
property, which we can remove.
Description
Store and JSDoc updates required to convert the
Screen
builder store to TypeScript.Launchcontrol
Convert the
Screen
builder store into TypeScript.