Expand category system to all top level items #1754

jcollins-g · 2018-09-12T21:08:12Z

I'm pretty excited about this PR, it has been a long time coming. I'm especially open to suggestions on how to do the style sheet for this, color selection and fonts, and so forth.

This PR expands category tag handling to all top level documented items. Changes how categories are configured -- the categoryOrder flag is now replaced with a more generic "categories" section in dartdoc_options.yaml. You can now specify user-visible names for categories. Categories can now have their own top level pages which are referred to as "APIs" and you can define where to load markdown to generate those pages.

Additionally, we generate json for all category data we load.

Screenshots from the test package:

Future work will need to make the categories able to span package boundaries more explicitly, but this should be good enough for Flutter and most pub packages.

devoncarew · 2018-09-14T17:47:41Z

This PR expands category tag handling to all top level documented items. Changes how categories are configured -- the categoryOrder flag is now replaced with a more generic "categories" section in dartdoc_options.yaml. You can now specify user-visible names for categories. Categories can now have their own top level pages which are referred to as "APIs"

🍾 very cool! I'll look through the PR now.

I'm especially open to suggestions on how to do the style sheet for this, color selection and fonts, and so forth.

The screenshot above looks good, and it's seems clear how to parse the information out when reading it.

The name of that top category - APIs - seems a bit odd, given that all of what you're seeing is API. I guess we're calling out the explicit semantic groupings of sub-apis defined in that package? I don't know of a better name; Topics? Categories? It could be possible to go without a name for that group, and just have them bare at the top level, but I'm not sure if that would look odd.

jcollins-g · 2018-09-14T18:00:25Z

@gspencergoog @InMatrix

devoncarew

lgtm; thanks for breaking this into separate commits (I didn't look at the 2nd commit - the test file regen one)

devoncarew · 2018-09-14T17:52:48Z

lib/resources/styles.css

+
+/* Colors for category based on categoryOrder in dartdoc_options.config. */
+.category.cp-0 {
+  background-color: #54b7c4


Where do these show up? Do we need the ability to change the background color of category labels?

Yes. Categories are tagged with cp-N where N is the position in category order, and this is used to give some color to the background of the category label. If a package has its own style sheet it can override those either using category order or the category name, which is also encoded into the style.

devoncarew · 2018-09-14T17:54:55Z

lib/src/dartdoc_options.dart

+
+  /// Canonical path of the markdown file used to document this category
+  /// (or null if undocumented).
+  final String documentationMarkdown;


nit: newline after the field

devoncarew · 2018-09-14T17:55:37Z

lib/src/dartdoc_options.dart

+  /// Canonical path of the markdown file used to document this category
+  /// (or null if undocumented).
+  final String documentationMarkdown;
+  CategoryDefinition(this.name, this._displayName, this.documentationMarkdown);


If _displayName is optional, perhaps used a named param?

I have avoided named parameters in constructors because there are situations where they aren't allowed. If you feel strongly I think it's safe to use it here.

I'm curious, what situation? I don't have a strong preference here. I prefer named params for optional values myself because I think it makes call sites more readable, and conveys intend about the param being optional.

devoncarew · 2018-09-14T17:57:01Z

lib/src/dartdoc_options.dart

+      YamlMap yamlMap, pathLib.Context pathContext) {
+    List<String> categoriesInOrder = [];
+    Map<String, CategoryDefinition> newCategoryDefinitions = {};
+    for (MapEntry entry in yamlMap.entries) {


Yeah, in the new world order, it's a lot more work to parse unstructured data.

Hopefully this design at least compartmentalizes the ugly into whatever class is being used with the DartdocOption.

Just calling out the additional work required in these situations -

devoncarew · 2018-09-14T17:59:47Z

lib/src/dartdoc_options.dart

@@ -102,6 +161,11 @@ class _OptionValueWithContext<T> {
          .toList() as T;
    } else if (value is String) {
      return pathContext.canonicalize(resolveTildePath(value as String)) as T;
+    } else if (value is Map<String, String>) {


I guess we're sure this is already a Map<String, String>, and not a Map<dynamic, dynamic>?

Yes; resolvedValue doesn't know what to do with other types. If a class used with fromYamlMap wants this support it has to deal with it itself. That could be changed, though.

devoncarew · 2018-09-14T18:44:55Z

lib/src/model.dart

+    }
+    _categoryNames = _categorySet.toList()..sort();
+    _subCategoryNames = _subCategorySet.toList()..sort();
+    if (_image == null) {


_image ??= '';?

devoncarew · 2018-09-14T18:47:47Z

lib/src/model.dart


+  // TODO: implement location


Perhaps make it a bit clearer what's missing here (what the todo refers to)?

this was added by an intellij fix. removed.

devoncarew · 2018-09-14T18:48:09Z

lib/src/model.dart

+  @override
+  String get location => pathLib.toUri(documentationFile.file.path).toString();
+
+  // TODO: implement locationPieces


actually forgot to implement this. That will fail an assert elsewhere if we ever warn on a subclass of this, so implemented

devoncarew · 2018-09-14T18:49:03Z

lib/src/model.dart

+  bool get isCanonical => categoryDefinition != null;
+
+  @override
+  String get kind => 'API';


I think we should noodle on a slightly better name for this.

devoncarew · 2018-09-14T18:49:37Z

lib/src/model.dart

@@ -5166,6 +5496,10 @@ class Package extends LibraryContainer

  @override
  Element get element => null;
+
+  // TODO: implement containerOrder


Is this todo still necessary?

no, deleted

InMatrix · 2018-09-14T19:56:14Z

The name of that top category - APIs - seems a bit odd, given that all of what you're seeing is API. I guess we're calling out the explicit semantic groupings of sub-apis defined in that package? I don't know of a better name; Topics? Categories? It could be possible to go without a name for that group, and just have them bare at the top level, but I'm not sure if that would look odd.

Topics sound good to me. We can also consider API Groups. Excited to see this landing. Do we need to do anything to enable this feature when generating dartdoc for Flutter? I assume some content authoring work might need to be done to create landing pages for each category and add more complete category information to classes.

jcollins-g · 2018-09-14T20:22:25Z

@InMatrix Flutter will need some work to make this happen; as is, you won't notice an immediate change. Ideally we'd create a tool that transforms the json already checked in to flutter manually, expanding it into comment blocks in the code.

InMatrix · 2018-09-14T20:33:32Z

@jcollins-g I'd be interested in experimenting with this feature locally. Let me know if you have some instructions on how to do it.

jcollins-g · 2018-09-14T21:09:13Z

@InMatrix The README is updated in this PR to hopefully give enough instructions to be worth trying it. I'll also upload a flutter branch.

jcollins-g · 2018-09-14T21:20:21Z

@InMatrix This is a sample flutter branch using the new category system. You'll have to activate a version of dartdoc including this PR, of course, for it to work.

https://github.com/flutter/flutter/compare/master...jcollins-g:categories-test?expand=1

jcollins-g added 2 commits September 12, 2018 13:42

Flatten non-doc changes

734b6b9

Rebuild test package docs

ad8181a

googlebot added the cla: yes Google CLA check succeeded. label Sep 12, 2018

Update changelog to pass test

4acaa20

jcollins-g requested a review from devoncarew September 13, 2018 16:31

devoncarew approved these changes Sep 14, 2018

View reviewed changes

Review comments

e6dcbff

jcollins-g added 2 commits September 14, 2018 14:06

Rename APIs to Topics

18c43eb

Rebuild test package docs

2660e09

jcollins-g merged commit 5e63ba8 into master Sep 17, 2018

jcollins-g deleted the dartdoc-categories-everything branch September 17, 2018 17:11

This was referenced Sep 18, 2018

External tool support #1756

Merged

Fix category order not recognized problem #1758

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expand category system to all top level items #1754

Expand category system to all top level items #1754

jcollins-g commented Sep 12, 2018 •

edited

Loading

devoncarew commented Sep 14, 2018 •

edited

Loading

jcollins-g commented Sep 14, 2018

devoncarew left a comment

devoncarew Sep 14, 2018

jcollins-g Sep 14, 2018

devoncarew Sep 14, 2018

jcollins-g Sep 14, 2018

devoncarew Sep 14, 2018

jcollins-g Sep 14, 2018

devoncarew Sep 14, 2018

devoncarew Sep 14, 2018

jcollins-g Sep 14, 2018

devoncarew Sep 14, 2018

devoncarew Sep 14, 2018

jcollins-g Sep 14, 2018

devoncarew Sep 14, 2018

jcollins-g Sep 14, 2018

devoncarew Sep 14, 2018

jcollins-g Sep 14, 2018

devoncarew Sep 14, 2018

jcollins-g Sep 14, 2018

devoncarew Sep 14, 2018

jcollins-g Sep 14, 2018

devoncarew Sep 14, 2018

jcollins-g Sep 14, 2018

InMatrix commented Sep 14, 2018

jcollins-g commented Sep 14, 2018

InMatrix commented Sep 14, 2018

jcollins-g commented Sep 14, 2018

jcollins-g commented Sep 14, 2018

Expand category system to all top level items #1754

Expand category system to all top level items #1754

Conversation

jcollins-g commented Sep 12, 2018 • edited Loading

devoncarew commented Sep 14, 2018 • edited Loading

jcollins-g commented Sep 14, 2018

devoncarew left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

InMatrix commented Sep 14, 2018

jcollins-g commented Sep 14, 2018

InMatrix commented Sep 14, 2018

jcollins-g commented Sep 14, 2018

jcollins-g commented Sep 14, 2018

jcollins-g commented Sep 12, 2018 •

edited

Loading

devoncarew commented Sep 14, 2018 •

edited

Loading