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

Reorganize tabs for Classlike #2764

Merged
merged 21 commits into from Feb 24, 2023
Merged

Reorganize tabs for Classlike #2764

merged 21 commits into from Feb 24, 2023

Conversation

vmishenev
Copy link
Member

@vmishenev vmishenev commented Dec 5, 2022

on #2749

@vmishenev vmishenev linked an issue Dec 5, 2022 that may be closed by this pull request
@vmishenev vmishenev marked this pull request as ready for review December 12, 2022 17:31
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.

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

@IgnatBeresnev
Copy link
Member

Some additional things that I noticed:

  • Anchors don't work in the preview. If you open Member + Extensions tab and then copy an anchor and follow it, it will reset selected tabs and not focus on the declaration
  • Found an empty Members tab with the Function header, for example in CoroutineScope class (see screenshot 1)
  • If a class has only extensions, should the tab be called "Members + Extensions" or just "Extensions"?
  • Should member and extension functions with the same name be merged like before or listed separately? (see screenshot 2)
  • Should constructors be listed separately as before or merged together like functions with the same name? (see screenshot 3)

2023-01-11_19-33-33

2023-01-11_23-05-14

2023-01-12_01-41-59

@vmishenev
Copy link
Member Author

These changes are obvious, but I would like to avoid a full redesign of tabbed content.
The HTML format is not stable yet. For example, in #2749 we had other tabs and had transformers that can add new tabs,
Besides, we would need to change GFM render as well.
And I'm not sure tabs should be used for classlikes. As for me, the checkbox Show extension is more obvious.

My changes in the existing content model are small.

@IgnatBeresnev
Copy link
Member

IgnatBeresnev commented Jan 13, 2023

but I would like to avoid a full redesign of tabbed content

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.

@vmishenev
Copy link
Member Author

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.

If a class has only extensions, should the tab be called "Members + Extensions" or just "Extensions"?

It is already discussed. "Members + Extensions"

Should member and extension functions with the same name be merged like before or listed separately?

Separately, so, at least, they lead to different pages.

Should constructors be listed separately as before or merged together like functions with the same name?

The behaviour has not changed in this PR.

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.

It's definitely getting better and easier to navigate / find usages 👍

@vmishenev
Copy link
Member Author

After a discussion, it was decided to get rid public API of ContentTabsExtra. The motivation is based on two statements:

  1. The tabs are needed only for HTML format. Other implemented formats do not use tabbed content
  2. The principle of decreasing new public API is applied, but some declarations of tabs are still in public API.

@vmishenev vmishenev self-assigned this Feb 22, 2023
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.

Very well done, thanks for addressing the comments! 👍 👍

Comment on lines +51 to +59
/**
* 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
*/
Copy link
Member

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 }
Copy link
Member

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

Copy link
Member Author

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

Wrap tab content
Fix css style
Revert public API in `DefaultPageCreator`
@vmishenev vmishenev merged commit 1040288 into master Feb 24, 2023
@vmishenev vmishenev deleted the 2763-reorganize-tabs branch February 24, 2023 15:44
IgnatBeresnev pushed a commit that referenced this pull request Feb 27, 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.

Reorganize tabs for class-like items
2 participants