Skip to content
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

feat: allow for plugins being passed down as props to <open-scd> #1486

Merged
merged 13 commits into from
May 7, 2024

Conversation

juancho0202
Copy link
Contributor

Closes #1418

Signed-off-by: Juan Munoz <juancho0202@gmail.com>
Signed-off-by: Juan Munoz <juancho0202@gmail.com>
Copy link
Member

@pascalwilbrink pascalwilbrink left a comment

Choose a reason for hiding this comment

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

The .plugins property is not adhering to the .plugins property structure of OpenSCD/core

Copy link
Contributor

@Stef3st Stef3st left a comment

Choose a reason for hiding this comment

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

This PR does met the acceptance criteria described in issue #1418 but most of the tasks described there have not been done. The Plugging mixin was supposed to be removed and a plugin interface should be exported in the Core's foundation file.

If these tasks are still valid then these changes need to be made. Otherwise the issue needs to be revised.

Signed-off-by: Juan Munoz <juancho0202@gmail.com>
Copy link
Contributor

@Stef3st Stef3st left a comment

Choose a reason for hiding this comment

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

Looks good, got a few (small) remarks and a few questions. As stated in the meetings, this PR will have a follow up to no longer merge the 2 plugin interfaces (#1503)

packages/core/foundation/plugin.ts Outdated Show resolved Hide resolved
packages/open-scd/.eslintrc.cjs Show resolved Hide resolved
juancho0202 and others added 4 commits April 15, 2024 11:18
Signed-off-by: Juan Munoz <juancho0202@gmail.com>
…h an editor plugin

Signed-off-by: Juan Munoz <juancho0202@gmail.com>
Signed-off-by: Juan Munoz <juancho0202@gmail.com>
@juancho0202 juancho0202 removed the request for review from Stef3st April 25, 2024 13:55
@juancho0202 juancho0202 dismissed Stef3st’s stale review April 25, 2024 13:57

@Stef3st is not part of the team and so I cannot request his time to do any reviews on open-scd

@juancho0202
Copy link
Contributor Author

I added a reactive property _sortedStoredPlugins in order to overcome a lifecycle problem I was experiencing by passing down to the Layout addon the sortedStoredPlugins getter like this:

<oscd-layout
  .host=${this}
  .doc=${this.doc}
  .docName=${this.docName}
  .editCount=${this.editCount}
  .plugins=${this.sortedStoredPlugins} // <-- When plugins were added to the localStorage this getter wouldn't force a rerender
>
</oscd-layout>

I ended up moving the storePlugins function into the OpenSCD class and triggering a change to the reactive prop after the localStorage is updated like this:

private get sortedStoredPlugins(): Plugin[] {
  return this.storedPlugins
    .map(plugin => {
      if (!plugin.official) return plugin;
      const officialPlugin = (officialPlugins as Plugin[])
        .concat(this.parsedPlugins)
        .find(needle => needle.src === plugin.src);
      return <Plugin>{
        ...officialPlugin,
        ...plugin,
      };
    })
    .sort(compareNeedsDoc)
    .sort(menuCompare);
}

@state()
_sortedStoredPlugins: Plugin[] = [];

private storePlugins(plugins: Array<Plugin | InstalledOfficialPlugin>) {
  localStorage.setItem('plugins', JSON.stringify(plugins.map(withoutContent)));
  this._sortedStoredPlugins = this.sortedStoredPlugins;
}

And then the reactive property _sortedStoredPlugins is passed down to the Layout like this:

<oscd-layout
  .host=${this}
  .doc=${this.doc}
  .docName=${this.docName}
  .editCount=${this.editCount}
  .plugins=${this._sortedStoredPlugins}
>
</oscd-layout>

@pascalwilbrink let me know if you agree with my approach here, these were some tricky merge conflicts. Thanks for the review.

Signed-off-by: Juan Munoz <juancho0202@gmail.com>
@pascalwilbrink pascalwilbrink reopened this May 7, 2024
@juancho0202 juancho0202 merged commit 01bcc01 into main May 7, 2024
10 checks passed
@juancho0202 juancho0202 deleted the 1418-plugins-as-params branch May 7, 2024 15:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Copy code from core's Plugging mixin into packages/open-scd/open-scd.ts
3 participants