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

Simplify parseHTML method of attributes to return the value itself #1863

Closed
philippkuehn opened this issue Sep 8, 2021 · 5 comments
Closed
Assignees
Labels
Type: Feature The issue or pullrequest is a new feature

Comments

@philippkuehn
Copy link
Contributor

The problem I am facing
At the moment it’s kind of repetitive to return an object within parseHTML.

addAttributes() {
  return {
    id: {
      default: null,
      parseHTML: element => {
        return {
          id: element.getAttribute('id'),
        }
      },
      renderHTML: attributes => {
        return {
          id: attributes.id,
        }
      },
    },
  }
}

The solution I would like
It would be great to just return the value because it’s already clear that it’s all about the attribute itself.

addAttributes() {
  return {
    id: {
      default: null,
      parseHTML: element => element.getAttribute('id'),
      renderHTML: attributes => {
        return {
          id: attributes.id,
        }
      },
    },
  }
}

Additional context
The initial idea was to be able to set multiple attributes within parseHTML but this some kind of unexpected and wasn’t needed until now so I think we can safely remove that feature.

But anyway, this would be a breaking change but I don't think it makes sense to build a fallback to the old behavior.

@philippkuehn philippkuehn added Type: Feature The issue or pullrequest is a new feature v2 labels Sep 8, 2021
@philippkuehn philippkuehn self-assigned this Sep 8, 2021
@hanspagel
Copy link
Contributor

I think that’s fine. 👍️

This breaking change would probably lead to issues that are not visible at the first glance. I think we should at least add a console.warn when an object is returned.

@tylerforesthauser
Copy link

Ah crud, I actually have a use-case for returning an object. I've got one attribute that influences another, mostly for our own backwards compatibility. So if certain conditions are met, I'm returning the values for both attributes in the single attribute's parseHTML method. With this change, is that still possible?

@philippkuehn
Copy link
Contributor Author

Oh no, this is not possible right now. And I’m not sure if we could support both. It becomes difficult to differ between these two cases because you can also save objects as attributes.

@hanspagel
Copy link
Contributor

Can’t you just add a parseHTML function for both attributes and do (nearly) the same in both functions? That should be an okayish workaround, or am I wrong?

const CustomParagraph = Paragraph.extend({
  addAttributes() {
    return {
      color: {
        parseHTML: element => element.getAttribute('data-color'),
      },
      legacyColor: {
        parseHTML: element => element.getAttribute('data-color'),
      },
    }
  },
})

@igor-ye
Copy link

igor-ye commented Dec 21, 2021

Hope you will not remove this feature in future versions. We also use objects stored in single attribute for custom Vue components properties. So we have only one custom node (componentNode) rendered as <component> tag with only two attributes - is and v-bind. And only one wrapping ComponentNodeEditor.vue for choosing editor type from the list:

<template>
	<node-view-wrapper class="component-editor">
		<a-select v-if="!componentName" :value="componentName" @change="componentSelected" placeholder="Select a block">
			<a-select-option v-for="e in blockEditors" :value="e">{{ e }}</a-select-option>
		</a-select>
		<component v-else :is="componentName" class="instance" :bindings="componentBindings" :updateProps="updateProps" />
	</node-view-wrapper>
</template>

So you don't need to write repetitive code in Node.create for every new component editor. Just one new MyNewCustomNodeEditor.vue which saves all props at once passing an object to updateProps.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Feature The issue or pullrequest is a new feature
Projects
None yet
Development

No branches or pull requests

4 participants