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

Support custom attributes for CSS and JS includes #2

Merged
merged 8 commits into from
Nov 17, 2023

Conversation

stefanseifert
Copy link
Member

@stefanseifert stefanseifert commented Jul 10, 2023

Fixes #1

Examples:

<sly data-sly-use.clientlib="/apps/wcm-io/wcm/ui/clientlibs/sightly/templates/clientlib.html"
    data-sly-call="${clientlib.css @ categories=['my-clientlib-category'],
    customAttributes=['attr1=value 1','data-attr2=5','attr3']}"/>
<sly data-sly-use.clientlib="/apps/wcm-io/wcm/ui/clientlibs/sightly/templates/clientlib.html"
    data-sly-call="${clientlib.js @ categories=['my-clientlib-category'],
    customAttributes=['attr1=value 1','data-attr2=5','attr3']}"/>

@stefanseifert stefanseifert marked this pull request as ready for review July 10, 2023 15:10
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

98.2% 98.2% Coverage
0.0% 0.0% Duplication

@gouravmakhija18
Copy link

gouravmakhija18 commented Oct 6, 2023

@stefanseifert - I am curious to know only one question ?.

Why to use "customAttributes" instead to have "categories" same as JSInclude?.

Note: We have predefined attribute for link rel and giving flexibility to have any attribute can pollute clientlibs inclusion in code too ?

e.g.
We have only below combinations available.

Case 1: "rel" attribute can have only "prefetch | preload | preconnect | dns-prefetch | prerender | modulepreload"
Case 2: "as" attribute can have only "style | script | document | font | image | video | fetch"
Case 3: "crossorigin" attribute implementation is already there in JSInclude.java but same is not defined in CSSInclude.java
Case 4: Not sure in JSInclude.java & CSSInclude.java file have support for only .css or .js file or other below mention combinations can be possible.

Different "rel" attribute variations

<link rel="prefetch" href="/style.css" />
<link rel="preload" href="/style.css" />
<link rel="preconnect" href="https://example.com" />
<link rel="dns-prefetch" href="https://example.com" />
<link rel="prerender" href="https://example.com/about.html" />
<link rel="modulepreload" href="/script.js" />

Different "as" attribute variations

<link rel="prefetch" href="/articles/" as="document">
<link rel="prefetch" href="/public/app.js" as="script">
<link rel="prefetch" href="/style.css" as="style" />

<link rel="preload" href="/assets/Adobe-Clean.woff2" as="font" type="font/woff2" crossorigin />
<link rel="preload" href="dummy.png" as="image" />
<link rel="preload" href="https://cdn.com/small-file.mp4" as="video" />
<link rel="preload" href="https://cdn.com/file_1.webm" as="fetch" />

Reference link: https://www.debugbear.com/blog/resource-hints-rel-preload-prefetch-preconnect

@stefanseifert
Copy link
Member Author

we currently have a set of HTML Standard attributes supported out of the box, with validation to ensure only valid property values are used, see https://wcm.io/wcm/ui/clientlibs/usage.html

for completely custom attributes like "data-contrast" as listed as example in #1 this PR can help.

maybe there as some HTML standard attributes we currently do not support out of the box we should add support for? the examples you are listing point in this direction - can you create a separate issues for this, that would be a separate PR.

@gouravmakhija18
Copy link

gouravmakhija18 commented Oct 6, 2023

we currently have a set of HTML Standard attributes supported out of the box, with validation to ensure only valid property values are used, see https://wcm.io/wcm/ui/clientlibs/usage.html

for completely custom attributes like "data-contrast" as listed as example in #1 this PR can help.

maybe there as some HTML standard attributes we currently do not support out of the box we should add support for? the examples you are listing point in this direction - can you create a separate issues for this, that would be a separate PR.

Thanks @stefanseifert for the quick response. I have created separate issue for above - #3

Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

96.7% 96.7% Coverage
0.0% 0.0% Duplication

@henrykuijpers
Copy link

Looks good to me, @stefanseifert !

@stefanseifert stefanseifert merged commit c008d4c into develop Nov 17, 2023
8 checks passed
@stefanseifert stefanseifert deleted the feature/custom-attributes branch November 17, 2023 11:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Setting additional (data-)attributes on the rendered script/link tag
3 participants