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

✨ Added support for attribute prefixes in PreactBaseElement #30639

Conversation

realPrimoh
Copy link
Contributor

@realPrimoh realPrimoh commented Oct 13, 2020

I added support for attribute prefixes in PreactBaseElement.

Usage:

AmpYoutube['props'] = {'params': {attrPrefix: 'data-param-'}};

<amp-youtube data-param-test="hello" />

This passes the object params = {'test': 'hello'} to the underlying Preact element.

Once this PR is merged, Bento documentation will be updated describing this attribute prefix usage.

@google-cla google-cla bot added the cla: yes label Oct 13, 2020
src/preact/base-element.js Show resolved Hide resolved
src/preact/base-element.js Outdated Show resolved Hide resolved
src/preact/base-element.js Outdated Show resolved Hide resolved
src/preact/base-element.js Show resolved Hide resolved
test/unit/preact/test-base-element-mapping.js Show resolved Hide resolved
test/unit/preact/test-base-element-mapping.js Show resolved Hide resolved
@realPrimoh realPrimoh marked this pull request as ready for review October 14, 2020 00:38
Copy link
Contributor

@dvoytenko dvoytenko 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 very close. Just a few more edge cases.

src/preact/base-element.js Show resolved Hide resolved
src/preact/base-element.js Show resolved Hide resolved
src/preact/base-element.js Outdated Show resolved Hide resolved
src/preact/base-element.js Outdated Show resolved Hide resolved
Copy link
Contributor

@dvoytenko dvoytenko left a comment

Choose a reason for hiding this comment

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

LGTM. Just two small nits.

src/preact/base-element.js Outdated Show resolved Hide resolved
src/preact/base-element.js Outdated Show resolved Hide resolved
@kristoferbaxter kristoferbaxter merged commit 00346cf into ampproject:master Oct 21, 2020
if (matchesAttrPrefix(attrib.name, def.attrPrefix)) {
currObj[
dashToCamelCase(
attrib.name.substring(def.attrPrefix.length, attrib.name.length)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: we can save a few bytes: attrib.name.slice(def.attrPrefix.length)

] = attrib.value;
}
}
if (Object.keys(currObj).length > 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: This is better done by storing a separate boolean value, doing Object.keys is expensive.

@@ -671,6 +675,20 @@ function usesShadowDom(Ctor) {
);
}

/**
* @param {null|string} attributeName
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this can ever be null? Was Closure complaining when it was just string?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, this was mainly so Closure doesn't complain. I don't think it can be null either.

Copy link
Contributor

Choose a reason for hiding this comment

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

MutationRecord, I believe, defines attributeName as nullable.

@samouri
Copy link
Member

samouri commented Oct 21, 2020

FMI, what was the motivating use-case for this feature?

@dvoytenko
Copy link
Contributor

@samouri It's very common for a Preact component's prop to be a map. E.g. style is a map and so could be many others. In HTML the same can be done with JSON serialization (rare) or with prefixed attributes. E.g. see Element.dataset.

@samouri
Copy link
Member

samouri commented Oct 22, 2020

@dvoytenko: are there any specific examples that we want to pass through to the Preact components? Is it mainly dataset or are there others?

@realPrimoh
Copy link
Contributor Author

@samouri For example, for amp-youtube, we are passing in data-param-* attributes as a params object to the Preact component. https://amp.dev/documentation/components/amp-youtube/#data-param-*

ed-bird pushed a commit to ed-bird/amphtml that referenced this pull request Dec 10, 2020
…ct#30639)

* Added support for data prefixes. Ex. data-param-*

* Added testing of attribute prefixes

* Addressed comments. Mutation check handled, tests more fleshed out.

* Removed startsWith to be consistent with master

* Addressed comments (handled def.attrPrefix === attrib.name)

* Addressed nits.

* Fixed gulp check-types error
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants