Skip to content
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

Add the link to homepage to the header if homepageLink is provided #3235

Merged
merged 8 commits into from
Nov 7, 2023

Conversation

whyoleg
Copy link
Contributor

@whyoleg whyoleg commented Oct 18, 2023

Fixes #2948

Added homepageLink parameter to DokkaBase plugin configuration. When setting it, icon with link to homepage will be shown near the theme switcher and search buttons in the header. Default icon is just platform-independent

@whyoleg whyoleg self-assigned this Oct 18, 2023
fun parseGithubInfo(link: String): Pair<String, String>? {
val (
_, // entire match
_, // optional 's' in http
Copy link
Member

@vmishenev vmishenev Oct 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think about https?://github\\.com/([\\w,\\-_]+)/([\\w,\\-_]+)/.* where s is no capturing group?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, agree, that should work, will fix it, thx!

Copy link
Member

@IgnatBeresnev IgnatBeresnev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For some reason, I don't see the icon in the header :( temporary preview link from GH actions

2023-10-23_15-04-14

Comment on lines 96 to 97
private fun calculateSourceUrlFromSourceLinks(): String? {
val githubLinkRegex = Regex("http(s)?://github\\.com/([\\w,\\-_]+)/([\\w,\\-_]+)/.*")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Optional as it's not that important atm: it looks like this function will be called for each page, so it could be optimized slightly:

  1. Afaik, each instance of Regex will compile the pattern under the hood, which takes some time, and it's generally advised to reuse it if it makes sense and is possible. I think in our case the Regex can be extracted into a companion or a top-level val or something, to be reused.
  2. We only need to calculate the return value once for the whole DokkaConfiguration (it will not change in-between invocations), and then we can reuse it. But I can't think of where to save this result off the top of my head. Maybe as a lateinit or a lazy property somewhere? Any ideas? :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If sourceUrl does not depend on a page, you can use buildSharedModel.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thx!

  1. extracted Regex to companion
  2. moved calculation of link to buildSharedModel

plugins/base/src/test/kotlin/renderers/html/HeaderTest.kt Outdated Show resolved Hide resolved
Comment on lines 51 to 54
override val context = MockContext(
override val context by lazy { context(configuration) }

fun context(testConfiguration: DokkaConfiguration): MockContext = MockContext(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(if you stick to HtmlRenderingOnlyTestBase and it's not reverted)

Was there some sort of a problem with it and this refactoring helped?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem is, while overall all of extensions are the same for all tests in file, configuration should be different per different tests - otherwise it's not possible to change some specific variables here.
F.e. in this case, I need to test output based on configuration.sourceSets[i].sourceLinks, and so configuration is different in each test function

@whyoleg
Copy link
Contributor Author

whyoleg commented Oct 24, 2023

For some reason, I don't see the icon in the header :(

Most likely this is because kotlinx.coroutines has no configuration of sourceLinks for root htmlMultiModule task.
Link to source: https://github.com/Kotlin/kotlinx.coroutines/blob/ed0cf7aa02b1266cb81e65e61b3a97b0e041a817/gradle/dokka.gradle.kts#L48
And also at least now it has sourceLinks configured for core project only (other modules don't have source in HTML generated, f.e. https://kotlinlang.org/api/kotlinx.coroutines/kotlinx-coroutines-android/kotlinx.coroutines.android/-handler-dispatcher/)

* no GitHub support by default
* use URL from base plugin configuration
* add integration test for the multi-module project that the homepage link exists everywhere
@whyoleg
Copy link
Contributor Author

whyoleg commented Oct 27, 2023

After internal discussion with @IgnatBeresnev some changes were applied:

  • replace GitHub image with some platform-independent icon (source of the link is in SVG: https://www.svgrepo.com/svg/416627/home-house-ui) - it's a temporary icon I believe
  • don't use sourceLinks to find homepage URL, because it not allows to fetch this configuration in reasonable way when building multi-module documentation - use base plugin configuration parameter

Question: Do we need to keep backward compatibility for DokkaBaseConfiguration?

Copy link
Member

@IgnatBeresnev IgnatBeresnev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍👍

plugins/base/src/main/kotlin/DokkaBaseConfiguration.kt Outdated Show resolved Hide resolved
@IgnatBeresnev
Copy link
Member

IgnatBeresnev commented Oct 30, 2023

it's a temporary icon I believe

Yep, we'll ask someone from the Kotlin Website team to help us with the icon and the styles, I've created a separate issue for that: #3300

@@ -73,6 +73,21 @@ class MultiModule0IntegrationTest : AbstractGradleIntegrationTest() {
"Expected moduleC being mentioned in -modules.html"
)

val indexHtmls = outputDir.walkTopDown().filter {
it.isFile && it.name == "index.html"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think all html files except navigation might be checked.

@@ -21,6 +21,9 @@
<@source_set_selector.display/>
</div>
<div class="navigation-controls">
<#if homepageLink?has_content>
<div class="navigation-controls--btn navigation-controls--homepage" id="homepage-link" role="button"><a href="${homepageLink}"></a></div>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have not checked but it seems to be <a href="${homepageLink}"> <div class="navigation-controls--btn navigation-controls--homepage" id="homepage-link" role="button"/></a>

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Im not an html-expert, but at least my variant works. If to switch tags, it doesn't work at my side (may be need to adapt more code)
I think it could be rechecked in scope of #3300

@whyoleg whyoleg merged commit 64cce58 into master Nov 7, 2023
11 checks passed
@whyoleg whyoleg deleted the source-link branch November 7, 2023 13:27
@whyoleg whyoleg changed the title Add the link to GitHub repo to the header if there are source links defined Add the link to homepage to the header if homepageLink is provided Nov 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dokka HTML: optionally add a custom link to source code homepage in navbar
3 participants