-
Notifications
You must be signed in to change notification settings - Fork 420
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
Reorganize tabs for Classlike #2764
Conversation
7e85362
to
e83f031
Compare
e83f031
to
2fc15fe
Compare
plugins/base/src/main/resources/dokka/scripts/platform-content-handler.js
Outdated
Show resolved
Hide resolved
plugins/base/src/main/kotlin/translators/documentables/DefaultPageCreator.kt
Outdated
Show resolved
Hide resolved
plugins/base/src/main/kotlin/translators/documentables/DefaultPageCreator.kt
Outdated
Show resolved
Hide resolved
88e3034
to
ea725b3
Compare
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 tried to wrap my head around how it worked before and how it's working now, but couldn't understand anything by just looking at this PR alone, so I started debugging
When I understood how it worked previously and how works in this PR, I tried to address things that I found confusing / not obvious / difficult to find, and tried to think of ways which would've made it easier.
I also tried to implement my suggestions to see if there were any problems/blockers that I didn't notice. Result can be found in #2810, it's a very rough draft, but it works for the most part. Hopefully it adds more context to my comments
I've commented on major things only since there's no point in commenting on code that might get deleted, so I'd like to have another look before merge
plugins/base/src/main/kotlin/translators/documentables/DefaultPageCreator.kt
Outdated
Show resolved
Hide resolved
plugins/base/src/main/kotlin/translators/documentables/DefaultPageCreator.kt
Outdated
Show resolved
Hide resolved
plugins/base/src/main/resources/dokka/scripts/platform-content-handler.js
Outdated
Show resolved
Hide resolved
plugins/base/src/main/kotlin/translators/documentables/DefaultPageCreator.kt
Outdated
Show resolved
Hide resolved
Some additional things that I noticed:
|
These changes are obvious, but I would like to avoid a full redesign of tabbed content. My changes in the existing content model are small. |
I'm not proposing a full redesign, although it would be nice. I do think that partial redesign is necessary though. I've outlined what I think could be improved and add clarity, with examples. It's unclear whether you're proposing to just disregard it all. |
I found time to implement some of the proposals, the most important ones.
It is already discussed. "Members + Extensions"
Separately, so, at least, they lead to different pages.
The behaviour has not changed in this PR. |
73a5fe6
to
92bdc27
Compare
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.
It's definitely getting better and easier to navigate / find usages 👍
plugins/base/src/main/kotlin/translators/documentables/DefaultPageCreator.kt
Outdated
Show resolved
Hide resolved
plugins/base/src/main/kotlin/translators/documentables/PageContentBuilder.kt
Outdated
Show resolved
Hide resolved
plugins/base/src/main/kotlin/translators/documentables/DefaultPageCreator.kt
Outdated
Show resolved
Hide resolved
plugins/base/src/main/kotlin/translators/documentables/DefaultPageCreator.kt
Outdated
Show resolved
Hide resolved
plugins/base/src/main/kotlin/translators/documentables/PageContentBuilder.kt
Outdated
Show resolved
Hide resolved
plugins/kotlin-as-java/src/test/kotlin/KotlinAsJavaPluginTest.kt
Outdated
Show resolved
Hide resolved
plugins/base/src/main/kotlin/translators/documentables/DefaultPageCreator.kt
Outdated
Show resolved
Hide resolved
After a discussion, it was decided to get rid public API of
|
a819cca
to
23d8bfb
Compare
23d8bfb
to
fb32411
Compare
fb32411
to
e884773
Compare
plugins/base/src/main/kotlin/translators/documentables/PageContentBuilder.kt
Outdated
Show resolved
Hide resolved
plugins/base/src/main/kotlin/translators/documentables/DefaultPageCreator.kt
Outdated
Show resolved
Hide resolved
plugins/base/src/main/kotlin/translators/documentables/DefaultPageCreator.kt
Outdated
Show resolved
Hide resolved
plugins/base/src/main/kotlin/translators/documentables/DefaultPageCreator.kt
Show resolved
Hide resolved
5facf89
to
1e76662
Compare
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.
Very well done, thanks for addressing the comments! 👍 👍
/** | ||
* Tabs themselves are created in HTML plugin since, currently, only HTML format supports them. | ||
* [TabbedContentType] is used to mark content that should be inside tab content. | ||
* A tab can display multiple [TabbedContentType]. | ||
* The content style [ContentStyle.TabbedContent] is used to determine where tabs will be generated. | ||
* | ||
* @see TabbedContentType | ||
* @see ContentStyle.TabbedContent | ||
*/ |
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.
🔥 🔥 🔥
|
||
private fun createTabsForClasslikes(page: ClasslikePage): List<ContentTab> { | ||
val documentables = page.documentables | ||
fun List<Documentable>.shouldDocumentConstructors() = !this.any { it is DAnnotation } |
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.
The same top-level public function is declared in DefaultPageCreator
- can it be used? Not sure if the copy-paste is intentional
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.
It will be unavailable when we have a separate HTML plugin
1e76662
to
e8086a6
Compare
6f9f6f1
to
d7a0317
Compare
(cherry picked from commit 1040288)
on #2749