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

Implemented client side hierarchy & expandable hierarchy section, #2744 #2749

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

mrfigg
Copy link
Contributor

@mrfigg mrfigg commented Oct 14, 2024

Resolves #2744.

Feedback & changes welcome.

Copy link
Collaborator

@Gerrit0 Gerrit0 left a comment

Choose a reason for hiding this comment

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

Some initial comments, without having played with it much locally yet

src/lib/output/plugins/HierarchyPlugin.ts Outdated Show resolved Hide resolved
src/lib/output/themes/default/assets/typedoc/Hierarchy.ts Outdated Show resolved Hide resolved
src/lib/output/themes/default/assets/typedoc/Hierarchy.ts Outdated Show resolved Hide resolved
src/lib/output/plugins/HierarchyPlugin.ts Show resolved Hide resolved
src/lib/output/themes/default/templates/reflection.tsx Outdated Show resolved Hide resolved
</>
<ul class="tsd-hierarchy tsd-full-hierarchy">
<li class="tsd-hierarchy-item">
<span>{context.i18n.theme_loading()}</span>
Copy link
Collaborator

Choose a reason for hiding this comment

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

This page should not need JS, including the full hierarchy in a place where users without it enabled can see it is useful.

Copy link
Contributor Author

@mrfigg mrfigg Oct 16, 2024

Choose a reason for hiding this comment

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

I was under the assumption that TypeDoc always requires JavaScript, as the entire navigation section is non-functional without it.

Assuming that was the case, this change was made to improve size on disk (otherwise the same data would be in both hierarchy.html and assets/hierarchy.js) and to make the hierarchy trees match. Currently interface hierarchies with multiple roots get partially rendered multiple times, where as the new rendering logic renders the entire tree once.

It would be possible to duplicate the rendering logic and make the hierarchy.html page static while fixing multi-root hierarchies, but that would still unnecessarily increase the file size if JavaScript is still required.

Should I proceed with this change or is it fine as is? Either way works for me.

Copy link
Collaborator

Choose a reason for hiding this comment

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

While the user experience is significantly degraded, it's still possible to navigate through the docs, the one link that exists in the navigation takes you to a page which lists all of its children, so it can be browsed via that method.

It will make the generated docs slightly larger, but including that information twice is acceptable, 500x times, less so :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed

@Gerrit0
Copy link
Collaborator

Gerrit0 commented Oct 20, 2024

This gets really confused with multiple inheritance, as is possible with interfaces:

class Foundation {}

export interface Base extends Foundation {}
export interface Branch extends Base {}

export interface Base2 {}

export interface Combined extends Branch, Base2 {}

image

@mrfigg
Copy link
Contributor Author

mrfigg commented Oct 20, 2024

Yea, I agree it's a bit confusing, but I'd argue it's more to do with an UL not being well suited to multi-parent graphs in general. That's actually working as intended. Actually you came across one of my test cases (test case 3 below). Is there an alternate layout that you would find more intuitive?

I came up with 3 rules for myself, but I'm open to changing them.

  1. Children of multiple parents should appear after their last parent both vertically and horizontally. (test case 1 & 2)
  2. Parents should appear before their top-most child both vertically and horizontally. (test case 3 & 4)
  3. Parents & children should be sorted alphabetically.

I'm guessing we want to tweak that last one.

Test Case 1:
/*
Graph:
╔═══╗                         ╔═══╗
║ A ║                         ║ G ║
╚══╤╝                         ╚╤══╝
   └──┐                     ┌──┘
     ╔╧══╗     ╔═══╗     ╔══╧╗
     ║ B ║     ║ D ║     ║ F ║
     ╚══╤╝     ╚╤═╤╝     ╚╤══╝
        └──┐ ┌──┘ └──┐ ┌──┘
          ╔╧═╧╗     ╔╧═╧╗
          ║ C ║     ║ E ║
          ╚═══╝     ╚═══╝

Desired Tree:
╔═══╗
║ A ║
╚═╦╤╝
  ║└────┐
  ║    ╔╧══╗
  ╠════╣ B ║
  ║    ╚══╤╝
  ║       └──────┐
  ║    ╔═══╗     │
  ╚════╣ D ║     │
       ╚╤╦╤╝     │
        │║└────┐ │
        │║    ╔╧═╧╗
        │╚════╣ C ║
        │     ╚═══╝
╔═══╗   │
║ G ║   └────────┐
╚═╦╤╝            │
  ║└────┐        │
  ║    ╔╧══╗     │
  ╚════╣ F ║     │
       ╚═╦╤╝     │
         ║└────┐ │
         ║    ╔╧═╧╗
         ╚════╣ E ║
              ╚═══╝
*/

export interface T1A {}
export interface T1B extends T1A {}
export interface T1C extends T1B, T1D {}
export interface T1D {}
export interface T1E extends T1D, T1F {}
export interface T1F extends T1G {}
export interface T1G {}
Test Case 2:
/*
Graph:
╔═══╗     ╔═══╗          ╔═══╗
║ A ║     ║ C ║          ║ F ║
╚══╤╝     ╚╤══╝          ╚══╤╝
   └──┐ ┌──┘                └──┐
     ╔╧═╧╗                    ╔╧══╗
     ║ B ║                    ║ G ║
     ╚═╤╤╝                    ╚╤╤═╝
       ││         ┌────────────┘│
       │└─────────┼──┐ ┌────────┘
       └────────┐ │  │ │
               ╔╧═╧╗╔╧═╧╗
               ║ D ║║ E ║
               ╚═══╝╚═══╝

Desired Tree:
╔═══╗
║ A ║
╚══╤╝
   └──────┐
╔═══╗     │
║ C ║     │
╚═╦╤╝     │
  ║└────┐ │
  ║    ╔╧═╧╗
  ╚════╣ B ║
       ╚═╤╤╝
╔═══╗    ││
║ F ║    │└────────┐
╚═╦╤╝    └───────┐ │
  ║└────┐        │ │
  ║    ╔╧══╗     │ │
  ╚════╣ G ║     │ │
       ╚╤╦╤╝     │ │
        │║└────┐ │ │
        │║    ╔╧═╧╗│
        │╠════╣ D ║│
        │║    ╚═══╝│
        └╫─────┐ ┌─┘
         ║    ╔╧═╧╗
         ╚════╣ E ║
              ╚═══╝
*/

export interface T2A {}
export interface T2B extends T2A, T2C {}
export interface T2C {}
export interface T2D extends T2B, T2G {}
export interface T2E extends T2B, T2G {}
export interface T2F {}
export interface T2G extends T2F {}
Test Case 3:
/*
Graph:
                    ╔═══╗
                    ║ E ║
                    ╚╤══╝
                  ┌──┘
               ╔══╧╗
               ║ D ║
               ╚╤══╝
             ┌──┘
╔═══╗     ╔══╧╗
║ A ║     ║ C ║
╚══╤╝     ╚╤══╝
   └──┐ ┌──┘
     ╔╧═╧╗
     ║ B ║
     ╚═══╝

Desired Tree:
╔═══╗
║ S ║
╚═╦═╝
  ║    ╔═══╗
  ╚════╣ S ║
       ╚═╦═╝
         ║    ╔═══╗
         ╚════╣ A ║
              ╚══╤╝
╔═══╗            │
║ E ║            │
╚═╦╤╝            │
  ║└────┐        └──────┐
  ║    ╔╧══╗            │
  ╚════╣ D ║            │
       ╚═╦╤╝            │
         ║└────┐        │
         ║    ╔╧══╗     │
         ╚════╣ C ║     │
              ╚═╦╤╝     │
                ║└────┐ │
                ║    ╔╧═╧╗
                ╚════╣ B ║
                     ╚═══╝
*/

export interface T3A {}
export interface T3B extends T3A, T3C {}
export interface T3C extends T3D {}
export interface T3D extends T3E {}
export interface T3E {}
Test Case 4:
/*
Graph:
╔═══╗          ╔═══╗
║ A ║          ║ D ║
╚═╤╤╝          ╚╤══╝
  │└───────┐ ┌──┘
  │       ╔╧═╧╗
  └───┐   ║ C ║
      │   ╚╤══╝
      │ ┌──┘
     ╔╧═╧╗
     ║ B ║
     ╚═══╝

Desired Tree:
╔═══╗
║ A ║
╚═╤╤╝
  │└────────────┐
  └──────┐      │
╔═══╗    │      │
║ D ║    │      │
╚═╦╤╝    │      │
  ║└────┐│      │
  ║    ╔╧╧═╗    │
  ╚════╣ C ║    │
       ╚═╦╤╝    │
         ║└────┐│
         ║    ╔╧╧═╗
         ╚════╣ B ║
              ╚═══╝
*/

export interface T4A {}
export interface T4B extends T4A, T4C {}
export interface T4C extends T4A, T4D {}
export interface T4D {}

@Gerrit0
Copy link
Collaborator

Gerrit0 commented Oct 27, 2024

This is probably why the original implementation of this block didn't try to render everything... it predates me.

Hmm... my main concern with this implementation is that it runs into an issue where it isn't easy to tell from the hierarchy which classes belong to which parts of the tree. For example in Test Case 1 the tree really makes it look like D inherits from A

I'm not actually sure there's any better option than what you've done besides rendering an actual graph, which I don't want to maintain, so don't want to go there... going to think about this a bit more, but will probably just go with it as is.

@Gerrit0
Copy link
Collaborator

Gerrit0 commented Nov 10, 2024

After entirely to much deliberation, I've decided that the right way to handle this is:

  1. Define the full hierarchy as starting at hierarchy roots, and including all children.
  2. If a child has already appeared in the page and has children, bail out early. If JS is enabled, add a button to expand it in place
  3. The "Expand" button on individual class/interface pages will simply include all hierarchy lists which contain the current reflection.

This isn't perfect, like the implementation here it means that showing the entire relevant hierarchy on a page accepts dropping any type arguments provided to parent classes but it avoids being completely unable to tell what members on the tree are children of others. (Preserving type arguments is a rather awful mess as really type parameters specified at the lower level should be passed through parent type parameters... but then if there's some conditional type or indexed type that ought to be handled specially... NO!),

Figuring this out required a fair amount of experimentation, so I have this half-done on a local branch based on yours, hopefully I can get it merged in tomorrow

Gerrit0 added a commit that referenced this pull request Nov 23, 2024
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.

Expandable Hierarchy Section
2 participants