-
Notifications
You must be signed in to change notification settings - Fork 88
Allow adding body scripts in a kobweb block (#299) #741
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
base: dev
Are you sure you want to change the base?
Allow adding body scripts in a kobweb block (#299) #741
Conversation
| body.add { | ||
| script { | ||
| src = "https://cdn.jsdelivr.net/npm/[email protected]/dist/js/bootstrap.bundle.min.js" | ||
| attributes["integrity"] = "sha384-geWF76RCwLtnZ8qwWowPQNguL3RmwHVBC9FhGdlKrxdiJJigb/j/68SIy3Te4Bkz" |
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.
Where did you get this sha from? Please document :)
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.
it's really fake/random data just to see what it looks like in the playground site
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.
Advice: When someone asks you a question on a code review, don't just think about is as a one-off question to answer. Think about it as a question that EVERY person who sees this code in the future might ask.
Therefore, when I get a question in a code review, I always ask myself if I should also add a comment, or change a variable or function name for clarity.
So in this case, that would look like:
body.add {
script {
src = "https://cdn.jsdelivr.net/npm/[email protected]/dist/js/bootstrap.bundle.min.js"
// Made up value, for testing:
attributes["integrity"] = "sha-xxxxxxxxxxxxxxxxxxxxxxxxxxxxx"
}
}Notice the added comment AND obviously fake sha value.
Of course, you don't always have to document a question; sometimes as a reviewer I might ask a dumb question because I was tired and not thinking straight, and you already feel like the code is obvious. However, I'd guess 80%+ I assume the question means something I did could be made clearer.
...ns/application/src/main/kotlin/com/varabyte/kobweb/gradle/application/extensions/AppBlock.kt
Show resolved
Hide resolved
…AFTER_SCRIPT, END) during index generation
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.
Hey I'm confused, what's going on here? I explicitly asked you NOT to add support for targeting different body locations but then you went ahead and added it. Please let me know how this miscommunication happened so I can be more careful next time. I don't want to waste your time.
Remember, adding more code just because we can is NOT an advantage. It's more code we have to maintain, more code we might potentially have to deprecate later if we made a mistake, and more area that bugs can slip in.
|
You're right, I'll be happy to switch back. I intended to do a fix which consists of exposing |
|
Yes, please switch back. I don't want to add extra code that we don't even know if anyone needs. All I wanted was a discussion to ensure that we weren't painting ourselves into a corner. |
…n via `body` lambda
…obweb-block' into Allow-adding-body-scripts-in-a-kobweb-block # Conflicts: # playground/site/build.gradle.kts # tools/gradle-plugins/application/src/main/kotlin/com/varabyte/kobweb/gradle/application/extensions/AppBlock.kt
… collections into a single `bodyElements` list and updating related processing logic
|
All should be good, feel free to review the overall changes and let me know if there are any updates needed |
5c34e2e to
e726ac5
Compare
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.
So I just noticed this PR misses communicating body blocks from library modules to the app module.
In other words, in playground, go into sitelib/build.gradle.kts, and type:
kobweb {
library {
index {
body.add { ... }
}
}
}Next, check out LibraryMetadata.kt, which has this code:
@Serializable
class LibraryMetadata(val index: Index) {
@Serializable
+ class Index(val headElements: String?)
}Follow through what headElements is doing and add support for bodyElements as well.
| buildTarget | ||
| src = basePath.prependTo(confInputs.script.substringAfterLast("/").prefixIfNot("/")), | ||
| scriptAttributes = indexBlock.scriptAttributes.get(), | ||
| bodyElements = bodyElements, |
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.
You had to add named parameters to everything because you put bodyElements out of order. Instead, just do this:
createIndexFile(
confInputs.title,
indexBlock.lang.get(),
headElements.applyUrlInterceptors(basePath).also { result ->
logger.lifecycle("Final <head> elements:")
logger.lifecycle("```")
result.elements.forEach { logger.lifecycle(" ${it.replace("\n", "")}") },
bodyElements.also { result -> ... }
)| return this.finalize() | ||
| } | ||
|
|
||
|
|
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.
Nit: Remove added dead space
| */ | ||
| fun serializeBodyContents(block: BODY.() -> Unit): String = | ||
| createHTML(prettyPrint = false, xhtmlCompatible = true).bodyFragment(block) | ||
|
|
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.
Super nit: Only one line of space between methods.
| body.add { | ||
| script { | ||
| src = "https://cdn.jsdelivr.net/npm/[email protected]/dist/js/bootstrap.bundle.min.js" | ||
| attributes["integrity"] = "sha384-geWF76RCwLtnZ8qwWowPQNguL3RmwHVBC9FhGdlKrxdiJJigb/j/68SIy3Te4Bkz" |
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.
Advice: When someone asks you a question on a code review, don't just think about is as a one-off question to answer. Think about it as a question that EVERY person who sees this code in the future might ask.
Therefore, when I get a question in a code review, I always ask myself if I should also add a comment, or change a variable or function name for clarity.
So in this case, that would look like:
body.add {
script {
src = "https://cdn.jsdelivr.net/npm/[email protected]/dist/js/bootstrap.bundle.min.js"
// Made up value, for testing:
attributes["integrity"] = "sha-xxxxxxxxxxxxxxxxxxxxxxxxxxxxx"
}
}Notice the added comment AND obviously fake sha value.
Of course, you don't always have to document a question; sometimes as a reviewer I might ask a dumb question because I was tired and not thinking straight, and you already feel like the code is obvious. However, I'd guess 80%+ I assume the question means something I did could be made clearer.
| attributes["crossorigin"] = "anonymous" | ||
| } | ||
| } | ||
| body.add { |
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 feel like this second body test is unnecessary and we should just remove it.
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.
BTW, I left some comments about the body script, but I'm thinking about deleting it. Because at this point, the code is fake but would require every Kobweb developer to develop bootstrap for no reason.
I think this feature is simple enough to just test it locally and then not check it in. At some point, I'll need to write actual unit tests for Kobweb sites, and that will be the right place to add fake body data like you are doing here.
| /** | ||
| * A list of element builders to add to the `<body>` of the generated `index.html` file. | ||
| * | ||
| * Elements are added after the main application script, making them suitable for: |
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 would just simplify this to:
Elements are added after the main application script. This positions them
at the end of the body block (where instructions usually tell you to add
them) and also ensures they run after the main script is already loaded.
You should normally use [ListProperty.add] ...
| ) | ||
|
|
||
| // Optional: Log body elements like we do for head ~ feel free to remove this if not needed | ||
| if (bodyElements.isNotEmpty()) { |
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.
This logic should be done in an also block like we do with headElements above.
| lang.convention("en") | ||
|
|
||
| // Initialize body as empty list (no defaults like head has) | ||
| body.set(emptyList()) |
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 not sure this is necessary?
Overview
This PR adds support for injecting custom
<body>elements (such as scripts, meta tags, and other HTML content) directly through the Kobweb application block configuration, enabling developers to add analytics, Bootstrap, and other scripts without modifying HTML templates.Closes #299
What's Changed
AppBlock.bodyconfiguration block for defining custom<body>content inbuild.gradle.ktsKobwebGenerateSiteIndexTaskto process and inject custom body elements during site generationserializeBodyContentsutility for creating and serializing custom<body>content blocksIndexTemplateto support custom<body>element injectionHtmlUtilwith body content serialization capabilitiesAPI Usage
Developers can now add custom body content in their
build.gradle.kts:kobweb { app { index { description.set("My awesome Kobweb app") // Add custom body elements using body.add {} body.add { script { src = "https://www.googletagmanager.com/gtag/js?id=GA_MEASUREMENT_ID" async = true } } body.add { script { src = "https://cdn.jsdelivr.net/npm/[email protected]/dist/js/bootstrap.bundle.min.js" attributes["integrity"] = "sha384-geWF76RCwLtnZ8qwWowPQNguL3RmwHVBC9FhGdlKrxdiJJigb/j/68SIy3Te4Bkz" attributes["crossorigin"] = "anonymous" } } body.add { script { unsafe { raw(""" // Custom analytics or other JavaScript console.log('Custom body script loaded'); """.trimIndent()) } } } } } }Use Cases
<body>tagImplementation Details
body.add {}blocks for adding multiple body elementsTesting
Breaking Changes
None - this is a purely additive feature that maintains full backward compatibility.