Allow adding body scripts in a kobweb block (#299)#741
Allow adding body scripts in a kobweb block (#299)#741kocheick wants to merge 10 commits intovarabyte:devfrom
Conversation
| body.add { | ||
| script { | ||
| src = "https://cdn.jsdelivr.net/npm/bootstrap@5.3.0/dist/js/bootstrap.bundle.min.js" | ||
| attributes["integrity"] = "sha384-geWF76RCwLtnZ8qwWowPQNguL3RmwHVBC9FhGdlKrxdiJJigb/j/68SIy3Te4Bkz" |
There was a problem hiding this comment.
Where did you get this sha from? Please document :)
There was a problem hiding this comment.
it's really fake/random data just to see what it looks like in the playground site
There was a problem hiding this comment.
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/bootstrap@5.3.0/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
bitspittle
left a comment
There was a problem hiding this comment.
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
bitspittle
left a comment
There was a problem hiding this comment.
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.
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.
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.
Super nit: Only one line of space between methods.
| body.add { | ||
| script { | ||
| src = "https://cdn.jsdelivr.net/npm/bootstrap@5.3.0/dist/js/bootstrap.bundle.min.js" | ||
| attributes["integrity"] = "sha384-geWF76RCwLtnZ8qwWowPQNguL3RmwHVBC9FhGdlKrxdiJJigb/j/68SIy3Te4Bkz" |
There was a problem hiding this comment.
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/bootstrap@5.3.0/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.
I feel like this second body test is unnecessary and we should just remove it.
There was a problem hiding this comment.
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.
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.
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.
I'm not sure this is necessary?
9100fe4 to
174e259
Compare
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/bootstrap@5.3.0/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.