Skip to content

Show single exports of resolved types in nav bar #280

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

Merged
merged 1 commit into from
Nov 24, 2018

Conversation

samselikoff
Copy link
Contributor

@samselikoff samselikoff commented Nov 15, 2018

If a module exports a single object, show that object under its resolved type in the side navigation.

Example from Mirage's docs: Collection is a single class export from the addon/orm/collection module.

image

Closes #267

@samselikoff samselikoff force-pushed the show-resolved-types-for-single-exports branch from 1fcb91d to 3854c62 Compare November 16, 2018 14:35
@samselikoff
Copy link
Contributor Author

@pzuraq @dfreeman Could I get a quick once-over of this when y'all have the time? This makes single-export modules appear a bit nicer in the nav index

Copy link
Contributor

@dfreeman dfreeman left a comment

Choose a reason for hiding this comment

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

Probably best to have Chris weigh in if he has the time, but from my somewhat-less-informed perspective this LGTM, particularly the added test coverage 🎉

(My only minor nit is that I'm not sure why NavigationIndexGenerator is a class instead of just a collection of functions since it has no state, but that's really all I have to complain about 😛)

Copy link
Contributor

@pzuraq pzuraq left a comment

Choose a reason for hiding this comment

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

👍 This seems good, definitely think that this is a solid default and makes class discoverability much better

@samselikoff
Copy link
Contributor Author

My only minor nit is that I'm not sure why NavigationIndexGenerator is a class instead of just a collection of functions since it has no state

@dfreeman good question... I don't really have a lot of experience "organizing free functions" (for lack of a better term), so I usually reach for classes since most of my work has been in OOP systems. I find them to be the easiest way to organize my thinking & refactor into small methods, and of course, there's always the chance that you'll add state later. (In Mirage I wrote the json:api graph serializer initially as a set of functions, then as a bunch of methods on a class, then added some state that tracked "has-been-serialized" models & collections, which I found drastically simplified some of the logic).

That being said I know I simply am not equipped to work on systems that are mostly comprised of functions, so those organizational patterns are just not familiar to me. I'm happy to take this opportunity to learn, if you have some pointers for me.

(One of the existing modules changed in this PR had a combination of a class export and free-floating functions as you can see here in master, but I personally find this sort of file hard to grok, since every function is at the same level. I don't know which ones are "higher-level" and "lower-level". And that's one of the reasons I extracted to a class with "the important stuff at the top.")

Again we can merge this now but I figured I'd write this up because it's something I've been interested on getting a better understanding of!

@samselikoff samselikoff merged commit a8500f5 into master Nov 24, 2018
@samselikoff samselikoff deleted the show-resolved-types-for-single-exports branch November 24, 2018 14:14
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.

API Docs: classes in modules
3 participants