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

Separating Schema label from code identifier #304

Merged
merged 6 commits into from
Nov 1, 2023

Conversation

hand-dot
Copy link
Collaborator

@hand-dot hand-dot commented Nov 1, 2023

ref: #303

CleanShot 2023-11-01 at 11 29 59@2x

@hand-dot hand-dot changed the title Separating Schema label from code identifier [TMP] Separating Schema label from code identifier Nov 1, 2023
@hand-dot hand-dot changed the title [TMP] Separating Schema label from code identifier Separating Schema label from code identifier Nov 1, 2023
@@ -219,7 +219,7 @@ describe('checkPlugins test', () => {
pdf: async () => {},
ui: async () => {},
propPanel: {
propPanelSchema: {},
schema: {},
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This fix is not directly related to this PR.


const pluginsSchemaTypes = Object.keys(plugins);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Stopped using the 'plugins' key.

@@ -62,7 +55,9 @@ const generate = async (props: GenerateProps) => {
continue;
}

const render = rendererRegistry[schema.type];
const render = Object.values(plugins).find(
(plugin) => plugin.propPanel.defaultSchema.type === schema.type
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There may be users who are only using the generator, but accessing plugin.propPanel.defaultSchema.type feels a bit awkward. Should it be accessible through plugin.type instead?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I might be overthinking this. If it doesn't feel awkward as is, I'd like to keep it that way.

Copy link
Collaborator

Choose a reason for hiding this comment

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

plugin.type would be syntactically cleaner, but plugin.propPanel.defaultSchema.type is not a problem to use 👍

@@ -1,11 +1,3 @@
import type { Schema, PDFRenderProps } from '@pdfme/common';
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We just use plugin.

This was a welcome side effect.
Thank you for making this proposal.

Copy link
Collaborator

@peteward peteward left a comment

Choose a reason for hiding this comment

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

this is looking a lot cleaner 👍

@@ -62,7 +55,9 @@ const generate = async (props: GenerateProps) => {
continue;
}

const render = rendererRegistry[schema.type];
const render = Object.values(plugins).find(
(plugin) => plugin.propPanel.defaultSchema.type === schema.type
Copy link
Collaborator

Choose a reason for hiding this comment

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

plugin.type would be syntactically cleaner, but plugin.propPanel.defaultSchema.type is not a problem to use 👍

@hand-dot hand-dot merged commit 5787079 into main Nov 1, 2023
2 checks passed
@hand-dot hand-dot deleted the separate-schema-label-from-code-identifier-303 branch November 9, 2023 00:35
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.

2 participants