-
Notifications
You must be signed in to change notification settings - Fork 52
feat: api ref doc sidebar optimization #1599
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: main
Are you sure you want to change the base?
Conversation
A new generated diff is ready to view.
|
A new generated diff is ready to view.
|
This comment has been minimized.
This comment has been minimized.
build.gradle.kts
Outdated
dokkaOutputDir.listFiles { file -> | ||
file.isDirectory && file.resolve("navigation.html").exists() | ||
}?.forEach { moduleDir -> |
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.
There should only be one navigation.html
file (in the root of dokkaOutputDir
) since we prune the duplicate navigation.html files during our build.
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.
Do we still want to prune them? Now we are trimming and using these navigations
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 didn't realize your navigation loader would use each module's navigation.html. Ok, that will work but we'll need to update our build process to not remove duplicate files (and make sure the docs artifact size doesn't increase significantly)
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.
Yep, I'll remove that. I did a quick estimation, the doc size should only increased by 300 - 400 MB
build.gradle.kts
Outdated
dokkaOutputDir.walkTopDown() | ||
.filter { it.isFile && it.name.endsWith(".html") } | ||
.forEach { file -> | ||
val updatedContent = file.readLines().filterNot { line -> | ||
line.contains("""scripts/navigation-loader.js""") | ||
}.joinToString("\n") | ||
|
||
file.writeText(updatedContent) | ||
} | ||
} |
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.
Instead of replacing the HTML content of every file, could we just create our own file navigation-loader.js and add it as a customAsset
? Would that override Dokka's default navigation loader script? It seems like a simpler solution
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.
No I don't think it will override Dokka's loader. The file from customAsset
will be put into images
folder like images/navigation-loader.js
The Dokka's own navigation-loader is in scripts
folders. So both files will be in the generated html.
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.
Oh ok, I see that here: https://github.com/Kotlin/dokka/blob/540fe084aadac0edb333350210ee22ee9901a3de/dokka-subprojects/plugin-base/src/main/kotlin/org/jetbrains/dokka/base/renderers/html/htmlPreprocessors.kt#L37
It sounds like a good feature request to Dokka if you can open one, but we can work around it for this PR
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.
Sounds good, I can open a request to Dokka
A new generated diff is ready to view.
|
This comment has been minimized.
This comment has been minimized.
A new generated diff is ready to view.
|
This comment has been minimized.
This comment has been minimized.
A new generated diff is ready to view.
|
This comment has been minimized.
This comment has been minimized.
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.
Should we do this for smithy-kotlin as well? The navigation sidebar isn't troublesome but for consistency it would be nice to do the same.
Yeah, I agree we should keep consistency. I am open to do for smithy-kotlin. |
|
A new generated diff is ready to view.
|
Affected ArtifactsNo artifacts changed size |
Issue #
Description of changes
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.