-
Notifications
You must be signed in to change notification settings - Fork 701
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
base: master
Are you sure you want to change the base?
Conversation
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.
Some initial comments, without having played with it much locally yet
</> | ||
<ul class="tsd-hierarchy tsd-full-hierarchy"> | ||
<li class="tsd-hierarchy-item"> | ||
<span>{context.i18n.theme_loading()}</span> |
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.
This page should not need JS, including the full hierarchy in a place where users without it enabled can see it is useful.
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 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.
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.
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 :)
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.
Changed
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.
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 {} |
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 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. |
After entirely to much deliberation, I've decided that the right way to handle this is:
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 |
Resolves #2744.
Feedback & changes welcome.