Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds a new silk-icons-lucide frontend module that provides generated Kotlin/Compose wrappers for Lucide icons in Kobweb projects, and wires it into the multi-project build and docs aggregation.
Changes:
- Added a new
:frontend:silk-icons-lucidemodule with a GradlegenerateIconstask that generatesLucideIcon+ per-icon composables fromlucide-icon-list.txt. - Added end-user documentation for loading Lucide JS and using the generated composables.
- Registered the new module in
settings.gradle.ktsand the docs aggregation tool.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/aggregate-docs/build.gradle.kts | Adds Lucide icon module to the set of projects whose docs are aggregated. |
| settings.gradle.kts | Includes the new :frontend:silk-icons-lucide Gradle module. |
| frontend/silk-icons-lucide/lucide-icon-list.txt | Adds the Lucide icon + deprecated-alias source list used for code generation. |
| frontend/silk-icons-lucide/build.gradle.kts | New module build + code generation logic for Lucide icon composables. |
| frontend/silk-icons-lucide/README.md | Usage docs for loading Lucide JS and rendering icons from Kotlin. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…port in build script.
…pt dependencies.
…Lucide icon metadata.
… `mutableSetOf` and sorting output explicitly.
…icons initialization.
|
Let me start with a high level comment first. This isn't an action item for change as much as an opportunity for a quick discussion. I decided to look how the other PR approached their solution, since I don't recall them having issues with when to call the // !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
// THIS FILE IS AUTOGENERATED.
//
// Do not edit this file by hand. Instead, update `lucide-icons.json` in the module root and run the Gradle
// task "generateIcons"
// !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
package com.varabyte.kobweb.silk.components.icons.lucide
import androidx.compose.runtime.Composable
import com.varabyte.kobweb.compose.ui.Modifier
import com.varabyte.kobweb.compose.ui.toAttrs
import com.varabyte.kobweb.compose.dom.svg.Circle
import com.varabyte.kobweb.compose.dom.svg.Line
import com.varabyte.kobweb.compose.dom.svg.Path
import com.varabyte.kobweb.compose.dom.svg.Polygon
import com.varabyte.kobweb.compose.dom.svg.Polyline
import com.varabyte.kobweb.compose.dom.svg.Rect
import com.varabyte.kobweb.compose.dom.svg.ViewBox
import com.varabyte.kobweb.silk.components.icons.createIcon
import com.varabyte.kobweb.silk.components.icons.IconRenderStyle
import org.jetbrains.compose.web.css.*
@Composable
private fun renderIcon(
elements: List<Pair<String, Map<String, String>>>,
modifier: Modifier = Modifier,
size: CSSLengthValue = 1.em,
strokeWidth: Number = 2,
color: CSSColorValue? = null
) {
createIcon(
viewBox = ViewBox.sized(24),
width = size,
renderStyle = IconRenderStyle.Stroke(strokeWidth),
attrs = modifier.toAttrs {
if (color != null) {
attr("stroke", color.toString())
}
attr("fill", "none")
attr("stroke-linecap", "round")
attr("stroke-linejoin", "round")
}
) {
elements.forEach { (elementType, attributes) ->
when (elementType) {
"path" -> {
Path {
attributes["d"]?.let { d(it) }
}
}
"circle" -> {
Circle {
attributes["cx"]?.let { cx(it.toDoubleOrNull() ?: 0.0) }
attributes["cy"]?.let { cy(it.toDoubleOrNull() ?: 0.0) }
attributes["r"]?.let { r(it.toDoubleOrNull() ?: 0.0) }
}
}
"rect" -> {
Rect {
attributes["x"]?.let { x(it.toDoubleOrNull() ?: 0.0) }
attributes["y"]?.let { y(it.toDoubleOrNull() ?: 0.0) }
attributes["width"]?.let { width(it.toDoubleOrNull() ?: 0.0) }
attributes["height"]?.let { height(it.toDoubleOrNull() ?: 0.0) }
attributes["rx"]?.let { rx(it.toDoubleOrNull() ?: 0.0) }
attributes["ry"]?.let { ry(it.toDoubleOrNull() ?: 0.0) }
}
}
"line" -> {
Line {
attributes["x1"]?.let { x1(it.toDoubleOrNull() ?: 0.0) }
attributes["y1"]?.let { y1(it.toDoubleOrNull() ?: 0.0) }
attributes["x2"]?.let { x2(it.toDoubleOrNull() ?: 0.0) }
attributes["y2"]?.let { y2(it.toDoubleOrNull() ?: 0.0) }
}
}
"polyline" -> {
Polyline {
attributes["points"]?.let { pointsStr ->
// Parse "x1,y1 x2,y2 ..." format
val pairs = pointsStr.split(" ").mapNotNull { point ->
val coords = point.split(",")
if (coords.size == 2) {
val x = coords[0].toDoubleOrNull()
val y = coords[1].toDoubleOrNull()
if (x != null && y != null) x to y else null
} else null
}
if (pairs.isNotEmpty()) {
points(*pairs.toTypedArray())
}
}
}
}
"polygon" -> {
Polygon {
attributes["points"]?.let { pointsStr ->
// Parse "x1,y1 x2,y2 ..." format
val pairs = pointsStr.split(" ").mapNotNull { point ->
val coords = point.split(",")
if (coords.size == 2) {
val x = coords[0].toDoubleOrNull()
val y = coords[1].toDoubleOrNull()
if (x != null && y != null) x to y else null
} else null
}
if (pairs.isNotEmpty()) {
points(*pairs.toTypedArray())
}
}
}
}
}
}
}
}
@Composable
fun LiAArrowDown(
modifier: Modifier = Modifier,
size: CSSLengthValue = 1.em,
strokeWidth: Number = 2,
color: CSSColorValue? = null
) {
renderIcon(
listOf("path" to mapOf("d" to "m14 12 4 4 4-4"), "path" to mapOf("d" to "M18 16V7"), "path" to mapOf("d" to "m2 16 4.039-9.69a.5.5 0 0 1 .923 0L11 16"), "path" to mapOf("d" to "M3.304 13h6.392")),
modifier, size, strokeWidth, color
)
}
// etc.In other words, they bake the SVG logic inside our code. Now that you've done it a different way and are closer to the code than I am, do you have any thoughts about this approach? |
|
We could use this solution too, it's more similar to how Lucide handles the different frameworks yes, I choose to took the simplest way first, if you agree, I can refactor to use this solution! |
|
Honestly, I was hoping you would push back and say your way is much better! Or list the pros and cons of both approaches? |
|
Oh also, one thing to know about this solution, is the way kotlin/js works that LoP found out was if you include one function inside a file, you kind of pull them all in. The dead code elimination step can't remove all the other functions, because they're treated as methods inside an anonymous class. That's why, if you look at our source code, under the SVG package, every single function is in its own file. If we went with a similar approach here, we'd have to generate one file per function. Otherwise, people including Lucide icons would get slammed with all the data. |
|
I think I'm going to refactor the code to use a similar code than the previous PR to fix those issues. Yeah I know that it would import all icons but for Font Awesome it's importing a font containing all of the icons too, and that's okay for many people. |
…st `generateIcons` Gradle task, and enhance SVG element parsing.
…JSON-based icon metadata in `silk-icons-lucide`.
… and updating README with `createIcon` API usage.
bitspittle
left a comment
bitspittle
left a comment
There was a problem hiding this comment.
Hey, so I finally started going through this, and I feel bad, because my gut it telling me this SVG approach is a mistake -- the sort of solution that an AI would happily push one into (because AIs don't fear regexes) but not what a human might have done on their own :).
My comments below are really at this point just concerns about readability and maintainability, nothing with functionality (which seems fine).
I'm not sure if it will be easier to continue moving forward through GitHub review or maybe you can reach out to me on Discord, so we can sort out any final points of friction.
I really appreciate you submitting this PR and I don't want the experience to be frustrating!
| // Path has unique code generation (string attribute with escaping) | ||
| if (tag == "path") { | ||
| val d = attributes["d"] ?: "" | ||
| return " Path {\n d(\"${escapeKotlin(d)}\")\n }" |
There was a problem hiding this comment.
Can we make this line more readable? Maybe use a multiline string?
return
"""
Path {
${escapeKotlin(d)}
}
"""| // Simple elements: open tag, emit each known attribute if present, close tag | ||
| simpleElementAttrs[tag]?.let { attrNames -> | ||
| return buildString { | ||
| append(" $tagCapitalized {\n") |
There was a problem hiding this comment.
It seems a little hard to follow to be hiding indentation like this across string constants.
One approach here is to put things behind a variable? private val indent = " " somewhere?
Another is to create data structures of values, collect them in a first pass, and then convert those data values into strings on a second pass, at which point you start inserting things into indented raw string templates.
Like, imagine a split like this:
class AttrInfo(val tag: String, val name: String)
// Parse attributes map and create a list of attr infos
buildString {
append("${indent]Path {")
for (attr in attrInfo) {
append("${indent}${indent}${attrInfo.tag.capitalize()}")
}
}In other words, replace all these smaller buildstring calls with a single buildstring call at the end, and capture things into rich typed classes. I can't say I'm 100% sure but I bet if you did it this would read a lot better / be easier to modify in the future if requirements change.
(This is a very rough equivalent of the coding approach of splitting the model and the UI, if you think about it)
There was a problem hiding this comment.
I've done something with a sealed class approach for different type of elements.
| "rect" to listOf("x", "y", "width", "height", "rx", "ry"), | ||
| ) | ||
|
|
||
| fun generateElementCode(tag: String, attributes: Map<String, String>): String { |
There was a problem hiding this comment.
It would be REALLY helpful to add a dev comment here (or maybe at the top of the class) that shows example input and example output. There's a lot of parsing code below and I could follow it a lot better if I understood the shape of the input coming in.
|
|
||
| for (line in iconsBlock.split("\n")) { | ||
| val trimmed = line.trim().removeSuffix(",") | ||
| val match = iconPattern.matchEntire(trimmed) ?: continue |
There was a problem hiding this comment.
Does this failed match mean we're skipping over an icon silently?
| ?: throw GradleException("Could not find 'icons' block in $generatedJsonFile") | ||
| val iconsBlock = iconsBlockMatch.groupValues[1] | ||
|
|
||
| val iconPattern = "\"([^\"]+)\"\\s*:\\s*\\[(.*)\\]".toRegex() |
There was a problem hiding this comment.
Example input here would be helpful. Regexes are hard to read for squishy humans like me.
| val elementsStr = match.groupValues[2] | ||
|
|
||
| val elements = mutableListOf<Pair<String, Map<String, String>>>() | ||
| val elemPattern = "\\{\"tag\":\"([^\"]+)\"(?:,\"attrs\":\\{([^}]*)\\})?\\}".toRegex() |
There was a problem hiding this comment.
Ugh I'm realizing now there are a LOT of regexes with this approach.
At the very least, collect these all to one place so we can reason about them together.
HOWEVER, regexes are almost always a code smell, and I have a growing feeling this is an indication that this approach is actually the wrong one, and your first approach may have been better. I would rather have a slightly inefficient payload that people won't even notice and simpler code than embedded SVGs that need to be regex parsed. The other approach should never really go wrong, because all we're doing is downloading their code; but if this approach goes wrong in the future, I really don't want to be on the hook for fixing or adding new regexes...
There was a problem hiding this comment.
I've used regexes since a few years and they aren't that hard in my opinion, also those are used in our own generated data, they'll never break by themselves. I've regrouped all of the regex patterns in one place and added comments to each of them.
There was a problem hiding this comment.
So it feels a bit like AI generated this. If I'm wrong let me know. Be careful because AIs happily generate text that humans might not want to read or edit later. At least here it seems you prompted it to keep it short.
The purpose of the README is really just for devs to understand the minimum of what's going on here and learn how to update things if users are asking for a new version in the future. You do have that in the latter half, everything after the ---, so that's good. But you can definitely drop the first part about the icon parameters, which can easily go stale and it's not necessary here.
Here's how I might fix it:
Support for integration of [Lucide icons](https://lucide.dev/) in your Kobweb project.
This directory contains a file called `lucide-icons.json`, which is parsed and used to generate code
used in this project.
To update it:
\`\`\`bash
./gradlew :frontend:silk-icons-lucide:fetchLucideIcons
\`\`\`
Once updated, run the following to regenerate the Kotlin source:
\`\`\`bash
./gradlew :frontend:silk-icons-lucide:generateIcons
\`\`\`
> [!NOTE]
> Each icon is generated as a separate Kotlin file to enable dead code elimination in Kotlin/JS -
> only the icons you actually use will be included in the final bundle.There was a problem hiding this comment.
I see, no problem, I've simplified the README!
frontend/silk-icons-lucide/README.md
Outdated
| Note that this directory contains a file called `lucide-icons.json`, which is parsed and used to generate code | ||
| used in this project. | ||
|
|
||
| Each icon is generated as a separate Kotlin file to enable dead code elimination in Kotlin/JS - |
There was a problem hiding this comment.
I'm glad this comment is here but I would also duplicate it in the build.gradle.kts task -- because that's probably where future me will go digging around first when trying to figure out the code.
| import kotlinx.html.dom.serialize | ||
| import kotlinx.html.head | ||
| import kotlinx.html.link | ||
| import kotlinx.html.script |
…rove clarity on icon generation steps.
…obwebGenerateSiteIndexTask.
…sk for `silk-icons-lucide`. Simplify element handling with `ElementInfo` sealed class and enhance JSON parsing with detailed regex patterns.
This pull request introduces a new package for integrating Lucide icons into Kobweb projects, along with documentation and an automated icon generation system.
Lucide Icon Integration and Automation
build.gradle.ktsscript infrontend/silk-icons-lucidethat parses a list of Lucide icons and generates corresponding Kotlin composable functions, including handling for deprecated icon aliases. This enables automatic code generation for icon wrappers and ensures the icon set stays up to date.frontend/silk-icons-lucide/README.mdexplaining how to integrate Lucide icons, load the required JS library, use the generated composables, and regenerate the icon source files after updating the icon list.