Collapse nested module names #907
Conversation
![sourcelevel-bot[bot]](https://avatars.githubusercontent.com/in/597?s=60&v=4){kind=small}
lib/ex_doc/retriever.ex
Outdated
|
|
||
| prefix -> | ||
| prefix = prefix <> "." | ||
| {prefix |> collapse_module_name(), String.trim_leading(title, prefix)} |
There was a problem hiding this comment.
Use a function call when a pipeline is only one function long
|
If I can give my 2 cents I'd like to say that I would like to see a few tweaks to this. In particular I do not like the tradeoff that you mentioned " I do agree that it isn't important to not emphasize nesting too much because strictly speaking the modules are not nested (there is no namespacing, just a common prefix). So I have two potential suggestions. Timex.Format Where everything in italics is in the lower contrast color that you're using in the screenshot (which I quite like). I think by keeping each row the same font size and weight that it doesn't appear that we are creating namespaces or a hierarchy. Also by having Also, perhaps when you click/tap on the |
|
@josevalim @teamon @whatyouhide @fertapric @OvermindDL1 @tmbb You've given your opinion on the previous iteration, please feel free to do so again :-) |
|
@davidsulc maybe we can mix both iterations? We add the Timex.Format as a group but we keep the abbreviation for each item in the group as seen here. |
|
@josevalim I think it might be better to keep the concepts orthogonal: groups are for "put this cohesive unit in its own place in the docs to draw attention to it", while collapsing is for "this module name is too long to display properly in the sidebar, and we're cutting off the part with the more specific info". As ExDoc beautifully mentions (https://hexdocs.pm/elixir/writing-documentation.html), documentation in Elixir is a first-class citizen (and thanks for that!). I feel this has led the community to put more effort in usable/user-friendly documentation which is a great dynamic to have. A key component to this, I think, is making writing readable docs as frictionless for developers as possible (i.e. prevent them from having to fight the tool). GroupingJust because you want to shorten certain module names in the sidebar (so they remain legible) doesn't necessarily imply you want to extract them into a group. After all, the way groups are displayed on their own implicitly suggests they are somehow special and/or form a cohesive unit. I think this should be driven by an explicit request from the documentation writer, not just automatically based on "these are long and have a common prefix". In other words, I'm worried of frustrating documentation writers who just want to shorten the display of their module names without necessarily having a group forced on them (which they might not want or agree to). Reusing the group mechanism wholesale would also mean that if a developer wants to have a section with a name matching a module prefix (e.g. Plug), they might have modules appear in that group (due to collapsing) that weren't part of the explicit list they provided which would go against the principle of least surprise. Granted, I can't think of a concrete example where this would be an issue (i.e. collapse module names with a common prefix, but only include a subset in an explicit group). Also how should ExDoc behave when the documentation writer defines a group, but some modules in there are too long to display? Extracting a sub-group from that group would also be somewhat counter-intuitive to me (especially because the extracted group might be displayed far away from its parent due to ordering). Finally, there are cases where module names are too long due to a prefix that is common to ALL modules (e.g. google_api_storage or phoenix_gen_socket_client. Putting these in a group when there are no modules that exist outside of that one single group would be a bit strange. Of course, those are edge cases but this entire PR is intended for edge cases :p Inserting a non-interactive nodeI think inserting an "empty module node" in the list (as suggested by @axelson) with the full name of the collapsed prefix (in cases where such a module doesn't exist in the project) would adequately address documentation clarity concerns while not introducing other issues. It also avoids surprises for the documentation writer, as although a new non-interactive module name may be added, the order/structure of the documentation remains as explicitly defined. This would also allow collapsing module names within module groups, if desired (while keeping them in their group). It also helps with "visual flow" as the font would be darker, there would be clear visual markers in the sidebar to quickly determine which modules are "grouped" under which prefix. In addition, as this approach is minimally intrusive with respect to the intentions of the documentation writer (with respect to order/structure/flow of the documentation), it would be possible down the road to introduce an opt-in flag of "collapse all modules that are too long". Please note that I'm just pointing out that we could, not necessarily that we should ;-) Next iterationUnless people are vehemently against it, I will move forward with adding the "non-interactive node" idea to this PR so we can discuss the pros/cons further. And I really hope I'm not coming across as "that guy who asks for feedback but just wants to hear his opinion in a different voice" :p |
|
Wow, that's a lot of effort put in here @davidsulc <3 <3 <3 Personally, I'm not a fan of achieved by this config: This would be easy enough to include in autogenerated projects (like google-* stuff). |
While that's true in the Google lib case (since the hierarchy is relatively flat, there's no need for collapsing beyond a single nesting level), X.Y.Z. gives your more info than just ... because you also get info about how deep the current nesting level is by looking at the line itself. As mentioned in the first comment here, I think indenting won't scale to cases where multiple levels must be nested. |
Likewise, plus there can be ambiguity. I entirely agree with @teamon here. You could even flatten multiple levels: If you really want to go multiple levels you can like this, or you can flatten the levels like: Both of the above should be supported based on how they define the grouping should occur. Regardless a tiny bit of indentation is hugely useful to differentiate such groupings, the abbreviations do not do that. |
I disagree. People would think it is clickable and it will be confusing, unless there is a visual cue. And if we are going to support both groups and nesting, we need to be careful as to how they will work together.
Yes, please. There is no chance we will support multiple levels. It just makes navigation deeper without larger benefits. |
I don't mean expandable levels, they all 'appear' top level, just indented as appropriate by a tiny amount. Unless someone starts doing it 12 layers deep then it shouldn't have issues going off the edge or so, literally just indent a tiny amount, a single character's width is fine. |
|
What do you guys think if we keep the discussion in one single place? As @davidsulc mentioned, this discussion is scattered into an issue, a closed PR and this PR. Ironically, I would create an entry in the Elixir Forum with a summary of what have been discussed, so we can collect more feedback. I think continuing the conversation there will help to keep the discussion organized (since I don't see we will reach an agreement in the next comments) and out of the issue tracker (or pull requests). Everybody ok? 😬 |
Fake modules are inserted in the list of modules to provide context for what the collapsed value means (i.e. would expand to). The sidebar will then be able to properly display the context information to the documentation user.
|
@fertapric If you still wish to get larger community feedback, please wait until I've been able to update this PR with a new iteration. I think I've found a nice solution and I'd like to avoid that what started as a way to address a very specific edge case spirals out of control into a "design by committee" situation. If more voices are going to add to the discussion, I would like them to at least do so against my next iteration which I hope (naively, perhaps 😅 ) will please most folks. |
| } | ||
|
|
||
| function hasPrefix (node) { | ||
| return node.title_prefix && node.title_prefix.length > 1; |
There was a problem hiding this comment.
Extra semicolon.
|
Ebert has finished reviewing this Pull Request and has found:
You can see more details about this review at https://ebertapp.io/github/elixir-lang/ex_doc/pulls/907. |
assets/js/events.js
Outdated
| var $target = $(e.target) | ||
| if ($target.hasClass('expand')) { | ||
| // the user might have clicked on the prefix span | ||
| var linkTag = $target.is('a') ? $target : $target.closest('a'); |
There was a problem hiding this comment.
Extra semicolon.
Nested modules have their name truncated, and are displayed beneath
a fake module in the sidebar for context. Module names inserted to
provide context for the nesting are not themselves truncated (i.e.
there is no recursion when adding context module nodes).
This feature is now managed by the `:group_modules_by_nesting` option
which replaces the previous `:collapse_nested_module_names`.
The `%ModuleNode{}` struct has been modified to support this:
* `title_prefix` and `title_collapsed` have been removed
* `title_collapsed` has been added
|
So, I've come full circle... I initially though the collapsing approach would nicely address the problem once context headers were added to the sidebar, and would be able to handle long prefix names by recursion. Having experimented with it a bit, it turned out unsatisfactory for reasons that probably aren't of interest here. I also explored displaying truncated context nodes, but that didn't work well either (from a usability point of view). Here's what I've settled on as the best approach: introduce a This option can be used in conjuction with The nesting context is updated as necessary: As you can see above, You can see the resulting docs here: These were generated with the following relevant options: Ping for feedback: @whatyouhide @fertapric @axelson Next steps?Assuming this revision finds some sort of consensus, I still need to add tests and get ebert bot's blessing: it seems the bot didn't run due to GitHub availability issues. Also: I've added the commits to this branch/PR for discussion/history purposes, let me know if you'd rather I create a new branch with a cleaner commit history (i.e. no detour through the collapsing experiment). |
|
Thanks @davidsulc ! This looks great, I have added some comments. The only nitpick I have is to remove the italic. I believe the italic puts the emphasis in the wrong place. I would maybe even consider removing the italic and the indentation mark, to keep the overall sidebar clean:
I am not a designer though :) but it feels the less markup aligns better with the current layout. |
|
@josevalim The markup can definitely be removed/reduced. The reason I added the arrow was because in cases where a LOT of modules share the same nesting level (e.g. the Google case), it's not immediately obvious you're not seeing the full module name (i.e. normal vs nested module):
Relying on indentation only means that you can only know you're seeing a nested value if you can see the beginning of the nesting (otherwise it's hard to know whether you're in an indented level or not). But maybe I'm over-thinking that 🤔 |
|
@davidsulc I think the google case is fine because on the initial scan of the project, you know that everything is nested, especially when you see the short names themselves. I think the issue is that, for a long list, you don't know which contexet you are right now, but none of them solve it. |
| {{/hasContent}} | ||
| {{else}} | ||
| <span title="{{node.title}}" class="context" aria-hidden="true"> | ||
| {{#if node.title_truncated}} |
There was a problem hiding this comment.
This title can never be truncated, right? As we don't support nesting.
There was a problem hiding this comment.
I believe you're right. I think this is an artifact from the recursive collapse experimentation, but I'll have to check to be sure.
lib/ex_doc/formatter/html.ex
Outdated
| defp process_unnested_node(%ModuleNode{} = module_node, acc) do | ||
| module_node | ||
| |> maybe_truncate_node_title(acc.prefixes_to_nest) | ||
| |> maybe_insert_nesting_context_node(acc) |
There was a problem hiding this comment.
I wonder if we should rather insert those nodes in the javascript side, like we do for groups. It is relatively easier to do it there. From the Elixir side, we will emit JS nodes with two information: the nesting and the nested_title. In the JS side, we check if the current node has a nesting, if so and it is different than the previous nesting, then we output the nesting header.
There was a problem hiding this comment.
I'd have to check if we can ensure consistent results if we do it on the js side. In particular, the "exceptions" section gets nesting "for free" by doing the node insertion in the elixir code. I.e. it's completely transparent whether a node is a module, an exception, or whether it belongs to a group: the nesting will behave the same. I need to check whether there's centralized processing in the js code or not (haven't really dived into that yet).
There was a problem hiding this comment.
It is exactly the sidebar_items.handlebars that you have changed. Here is the code for the group: https://github.com/elixir-lang/ex_doc/pull/907/files#diff-d85f51765b385ca15bb7242d5936ac89R3
I propose something like very similar to that, in pseudo code it would be:
{{#newNesting ../this node.nesting}}
<li class="nesting">{{node.nesting}}</li>
{{/newNesting}}
However, keep in mind that, if we start a new group and the first item of the group has a nesting, we need to emit the nesting too. So maybe you also need to pass the group as argument. Alternatively, when you check for newGroup and the group change, you just reset the parent "last nesting" value.
Added by mistake during development
Recursive nesting isn't supported
* don't italicize nesting context module's name * indent nested modules and remove preceding icon
They don't need to be added to the list of nodes: <li> elements can be added to the UI as new nesting contexts are encountered.
|
@josevalim I've updated my PR, and I think I've addressed all your comments. I've also updated the resulting docs: modified Timex and Google storage Let me know if there's anything else I can improve. |
| <a href="{{node.id}}.html" title="{{node.title}}" class="expand">{{node.title}}</a> | ||
| {{#nestingContextChanged ../this node.nested_context}} | ||
| <li class="nesting-context" title="{{node.nested_context}}" aria-hidden="true">{{node.nested_context}}</li> | ||
| {{/nestingContextChanged}} |
There was a problem hiding this comment.
Sorry for the super minor nitpick but can we please keep the newGroup and nestingContextChange function names in sync? We can either call both groupChanged and nestingChanged or newGroup and newNesting, I would just like to keep them in sync. :)
There was a problem hiding this comment.
👍 I'll update them to ...Changed. I didn't name it newNesting because it's confusing: the nesting isn't necessarily "new" as there can be duplicates.
lib/ex_doc/formatter/html.ex
Outdated
| end | ||
| end | ||
|
|
||
| defp add_nesting_info([], _), do: [] |
There was a problem hiding this comment.
I think we can move this to the retriever now, so the modules already built with the nesting information. :)
There was a problem hiding this comment.
Once you move it to the retriever, we can add some unit tests too!
* renamed newGroup -> groupChanged * renamed nestingContextChanged -> nestingChanged These were normalized to the ...Changed form, as it fit better than new...: the nesting context isn't necessarily new (duplicates can happen).
|
I think I like the marker before the nested modules |
|
@josevalim I've improved my PR with your suggestions |
|
❤️ 💚 💙 💛 💜 |
| * `:groups_for_extras`, `:groups_for_modules`, `:groups_for_functions` - See next sections | ||
| * `:group_modules_by_nesting` - A list of atoms (e.g. `Foo.Bar`) that will be truncated |
There was a problem hiding this comment.
Minor nitpick: should we say here "A list of module names (e.g. Foo.Bar)"? The example says Foo.Bar instead of :"Elixir.Foo.Bar"
|
Given that |
Uses a combination of `nest_modules_by_prefix` and `groups_for_modules` ex_doc options (docs https://hexdocs.pm/ex_doc/Mix.Tasks.Docs.html#module-nesting) to improve the generated ex_doc sidebar to be more readable. I do wish that ex_doc would give us some more options to control how the sidebar looks, in particular I wish there was an indicator to make it more obvious that a module is nested (currently there's just a `.`). In an earlier version of the PR there was a nice visual indicator: elixir-lang/ex_doc#907 (comment) Also updates credo and ex_doc
Uses a combination of `nest_modules_by_prefix` and `groups_for_modules` ex_doc options (docs https://hexdocs.pm/ex_doc/Mix.Tasks.Docs.html#module-nesting) to improve the generated ex_doc sidebar to be more readable. I do wish that ex_doc would give us some more options to control how the sidebar looks, in particular I wish there was an indicator to make it more obvious that a module is nested (currently there's just a `.`). In an earlier version of the PR there was a nice visual indicator: elixir-lang/ex_doc#907 (comment) Also updates credo and ex_doc
Uses a combination of `nest_modules_by_prefix` and `groups_for_modules` ex_doc options (docs https://hexdocs.pm/ex_doc/Mix.Tasks.Docs.html#module-nesting) to improve the generated ex_doc sidebar to be more readable. I do wish that ex_doc would give us some more options to control how the sidebar looks, in particular I wish there was an indicator to make it more obvious that a module is nested (currently there's just a `.`). In an earlier version of the PR there was a nice visual indicator: elixir-lang/ex_doc#907 (comment) Also updates credo and ex_doc
7 participants
Here's an attempt at addressing long module names in the sidebar. Prior discussion has shown that there is no consensus on the best approach to address the issue of long module names getting clipped in the sidebar, but I believe this proposal is the "least bad" solution (and I'll cover why below).
Context
As you evaluate this, please bear in mind the relevant issue was opened in February this year and hasn't seen activity since July, so this is likely an infrequent issue within the community (i.e. if this PR gets merged it hopefully won't besmirch the docs you use daily). In addition, don't forget this option is opt-in: if long module names aren't getting clipped within the sidebar, there's no reason to use it.
Before getting in the thick of things, here's what we're talking about:
As you can see, modules with long names have their right hand portion cut off, which is especially unfortunate as that is the most specific portion of the namespace.
This PR proposes to allow users to specify a set of prefixes that would get "collapsed" in order to favor showing the rightmost namespace sections (to the detriment of the leftmost sections, which are typically of less interest). Here's what the documentation for the same project would look like:
This result was obtained by passing in the following option:
Why I like it
The difference in font opacity signals that the prefix is "different" than the uncollapsed portion. Even if the user doesn't realize that the prefix is collapsed from the context, the affordance indicated by the font difference will typically lead them to hover the module name, whereupon they will see the full title name displayed.
The darker font of the collapsed prefixes gives a visual indentation communicating the concept of module nesting.
Keeps the straightforward nesting currently used by ExDoc: modules are all on a single level (similar to how they exist within Elixir) with indentation reserved for things that are actually contained within the module (functions, types, etc.).
Of course, this solution isn't perfect either: in the example above,
T.F.DateTime.Formattergives the visual impression that it is nested withTimex.Duration. This is a trade-off made by the documentation writer, as it enabled the subsequent formatters to be visible within the sidebar. Ultimately, this option puts the onus on the developer to make the trade-offs that best suits their specific use case and makes their documentation most user-friendly. In my opinion, that's as it should be: the developer is precisely who should be empowered to make those trade-offs.Shortcomings of alternative approaches
Grouping
My initial attempt sought to leverage the existing
:groups_for_modulesfunctionality. Having given this more thought, I believe this approach was misguided. There has to be a visual cue that something "unusual" is being done to accommodate excessively long module names so the user can process the information appropriately.I don't think there's a really good way to do this without needlessly increasing the complexity of front-end documentation code (either by making the current group mechanism more complex, or introducing a new one). Either you have a bold/uppercase/non-clickable title meaning the "root" module has to be repeated below the title, or you need a group title that displays like a normal module, which means that there now needs to be some visual differentiation for the "nested" modules.
In addition, sorting would have to be handled explicitly to ensure "groups by nesting" are displayed in the proper place (i.e. alphabetically, where the user expects them).
Nesting with indentation
Another idea was nesting modules in the sidebar with (e.g.) nested
ullists. I think this idea is also problematic as it just punts the problem down the line: nested modules will eventually indent far enough that the functions they contain will be clipped in the sidebar and we'd be back at square one.In addition, I think indenting nested modules sends the wrong message, namely that nested modules are "within" parent modules (i.e. there's a module hierarchy). This is already a source of confusion for newcomers to the language, and I don't think the documentation system should add to the confusion.
Finally, using nesting and indentation for modules in the sidebar muddies the waters: currently, the sidebar docs has a nice semantic where modules are all on the same level (i.e. "equal"), and clicking on a module will expand to display an indented list of things that actually exist within the module. This is a really nice analogy, and I think it's worth the effort to keep it that way if we can.
Conclusion
Now that I've said my piece, please feel free to provide feedback, counter points, etc. As you'll probably have noticed, this PR isn't ready for merging yet: it contains no tests. This is because I'd rather wait until the dust settles on what the community thinks/wants so effort isn't invested on things that aren't a good fit.