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 member icons to navigation menu #2578

Merged
merged 7 commits into from
Jul 29, 2022
Merged

Add member icons to navigation menu #2578

merged 7 commits into from
Jul 29, 2022

Conversation

IgnatBeresnev
Copy link
Member

@IgnatBeresnev IgnatBeresnev commented Jul 20, 2022

@IgnatBeresnev IgnatBeresnev mentioned this pull request Jul 20, 2022
@IgnatBeresnev
Copy link
Member Author

Documentables with Java sources will have Java-styled icons, otherwise Kotlin-styled icons will be displayed.

Also added handling of long text links and package names in the navigation tree, it's displayed properly now, with icon always on the left (see this class for instance)

Current pre-review version for a mixed language project: https://ignatberesnev.github.io/static/dokka/icons/alchemist-java/alchemist-incarnation-biochemistry/it.unibo.alchemist.model.interfaces/-environment-supporting-deformable-cells/index.html

@@ -191,56 +191,6 @@ class KotlinEnumsTest : BaseAbstractTest() {
}
}

@Test
fun `should preserve enum source ordering for navigation menu`() {
Copy link
Member Author

@IgnatBeresnev IgnatBeresnev Jul 23, 2022

Choose a reason for hiding this comment

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

Enum entries have been dropped from the navigation menu.

Also removed duplicated class KotlinEnumTest - no differences in terms of tests

@@ -57,4 +68,50 @@ class JavaEnumsTest : BaseAbstractTest() {
}
}
}

@Test
fun `should mark synthetic functions generated for Kotlin as obvious`() {
Copy link
Member Author

Choose a reason for hiding this comment

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

Looks like some classes got duplicated for some reason, probably due to automatic conflict resolution. Deleted JavaEnumTest and moved this test to JavaEnumsTest, that was the only difference

import org.jetbrains.dokka.plugability.DokkaContext
import org.jetbrains.dokka.transformers.pages.PageTransformer

open class NavigationPageInstaller(val context: DokkaContext) : NavigationDataProvider(), PageTransformer {
Copy link
Member Author

@IgnatBeresnev IgnatBeresnev Jul 23, 2022

Choose a reason for hiding this comment

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

The class was getting a bit too long for htmlPreprocessors.kt, extracted it. chooseNavigationIcon method is new

Copy link
Member

Choose a reason for hiding this comment

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

NavigationPageInstaller is a html preprocessor so it should be in htmlPreprocessors.kt.
Extracted NavigationDataProvider is ok .

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think it necessarily should be in the same file, sooner or later it'll grow so big that all of them will have to be extracted into their own ones. Imho, grouping classes by features rather than domain is easier for navigating around

Doesn't make a huge difference now, so returned it

Copy link
Member

@vmishenev vmishenev Jul 28, 2022

Choose a reason for hiding this comment

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

My concern: If I open htmlPreprocessors.kt, I expect to see ALL preprocessors there.

Imho, grouping classes by features rather than domain is easier for navigating around

Anyway now there are two different grouping simultaneously. It is misleading.

@IgnatBeresnev IgnatBeresnev marked this pull request as ready for review July 23, 2022 19:00
Copy link
Member

@vmishenev vmishenev left a comment

Choose a reason for hiding this comment

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

So I am waiting source dependent icons.
And it would be great to combine the icons into a SVG sprite.

Copy link
Member

@vmishenev vmishenev left a comment

Choose a reason for hiding this comment

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

Have you checked it in a mobile browser!?

import org.jetbrains.dokka.plugability.DokkaContext
import org.jetbrains.dokka.transformers.pages.PageTransformer

open class NavigationPageInstaller(val context: DokkaContext) : NavigationDataProvider(), PageTransformer {
Copy link
Member

Choose a reason for hiding this comment

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

NavigationPageInstaller is a html preprocessor so it should be in htmlPreprocessors.kt.
Extracted NavigationDataProvider is ok .

@IgnatBeresnev
Copy link
Member Author

Have you checked it in a mobile browser!?

I did, it works and doesn't look worse than before. Is there something wrong with it?

@vmishenev
Copy link
Member

I did, it works and doesn't look worse than before. Is there something wrong with it?

No, I have not tested it there.

@IgnatBeresnev IgnatBeresnev merged commit 7a875ee into master Jul 29, 2022
@IgnatBeresnev IgnatBeresnev deleted the navigation-icons branch July 29, 2022 12:32
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.

2 participants