-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
feat(core): support custom html elements in head tags #11571
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
Conversation
✅ [V2]
To edit notification comments on pull requests, go to your Netlify project configuration. |
slorber
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
I'm adding a little unit test
Also unfortunate that the tag validation logic is spread in 2 different places, but that can be refactored in a separate refactor PR.
| An array of tags that will be inserted in the HTML `<head>`. The values must be objects that contain two properties; `tagName` and `attributes`. `tagName` must be a string that determines the tag being created; eg `"link"`. `attributes` must be an attribute-value map. When custom html elements are needed, set `customElement: true`. | ||
| - Type: `{ tagName: string; attributes: Object; }[]` | ||
| - Type: `({ tagName: string; attributes: Object; } | { tagName: string; attributes?: Object; customElement: true; })[]` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm going to avoid the union type in docs. Although more accurate, it could make the docs harder to read for the usual case where devs are not using custom elements
Although we can be flexible, I'm not sure what could be the value of adding a custom element in head if it doesn't have any attribute. It would only be a "marker" somehow, but it's not really your use-case. I think it's fine if we keep documenting it as if the attributes were required (the object can be empty anyway)
|
Not sure why this PR doesn't trigger our CI jobs Maybe it's because you created the branch from main (it's better to create a dedicated branch in the future). Anyway, I fixed the test failures and will merge |
|
Thanks for the additions, suggestions, and advice — I’ll make sure to apply them next time :) |
|
np 👍 The GH actions problem is something else, looks to apply to many Meta repo but nobody told me anything 😅 |
Pre-flight checklist
Motivation
see #11570
Test Plan
How to test this? I checked it locally by editing
docusaurus.config.tswith all possible combinations...Tested Configurations
Valid Configs: ✅
Invalid Configs ❌
Test links
Deploy preview: https://deploy-preview-_____--docusaurus-2.netlify.app/
Related issues/PRs
#11570