Skip to content

Conversation

@david-monichi
Copy link
Collaborator

Integrate Wizarding API V3 from OpenEnergyTools to OpenSCD

As external project the PR can't be marked as draft.

@david-monichi david-monichi requested a review from trusz February 25, 2025 10:31
@senzacionale senzacionale force-pushed the feat/wizarding-v3 branch 2 times, most recently from f21f485 to e2bb55b Compare February 25, 2025 11:02
@Sander3003
Copy link
Member

@david-monichi is this one ready to review?

Copy link
Member

@trusz trusz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a few nitpickings

Comment on lines +92 to +95
this.updateComplete
.then(() => this.wizardUI.updateComplete)
.then(() => this.wizardUI.dialog?.updateComplete)
.then(() => this.wizardUI.dialog?.focus());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the small cleanups too :)

Comment on lines 101 to 110
this.addEventListener('wizard', this.onWizard as EventListener);
this.addEventListener('oscd-edit-wizard-request', (e: Event) =>
this.onWizardRequest(e as CustomEvent<EditWizardRequest>)
);
this.addEventListener('oscd-create-wizard-request', (e: Event) =>
this.onWizardRequest(e as CustomEvent<CreateWizardRequest>)
);
this.addEventListener('oscd-close-wizard', () => this.onCloseWizard());
this.addEventListener('editor-action', () =>
this.wizardUI.requestUpdate()
this.wizardUI?.requestUpdate()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

couldn't we put the event listener on the HTML elements?
something along these lines:

render(): TemplateResult {
    return html`
        ${ifImplemented(super.render())}
        <wizard-dialog
          .wizard=${this.workflow[0]?.() ?? []}
          @oscd-edit-wizard-request=${this.onWizardRequest}
          @oscd-close-wizard=${this.onCloseWizard}
        >
        </wizard-dialog>
      `;
    }

This way Litjs can handle their lifecycle.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's possible. I'll test it and make the change.

} else if (wizard === null) {
this.workflow.shift();
} else if (wizard) {
subwizard ? this.workflow.unshift(wizard) : this.workflow.push(wizard);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there a reason why it is not just another 'if-else'?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No special reason. I will change it.

Comment on lines +95 to +98
this.updateComplete
.then(() => this.wizardUI.updateComplete)
.then(() => this.wizardUI.dialog?.updateComplete)
.then(() => this.wizardUI.dialog?.focus());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

again, thank you for the clean up :)

@trusz
Copy link
Member

trusz commented Mar 12, 2025

I see the deployment has not been tested. I'll try it locally first

@senzacionale
Copy link

senzacionale commented Mar 30, 2025

Hello @trusz
I update code with your suggestions. And also merge PR 1643. I've added test plugin.

Working url: http://146.247.24.58/

Please take a look. Thank you

@senzacionale
Copy link

Hello @trusz
Did you have time to check this PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants