Adding apiDocumentationEditor to collection and settings page.

[sebbel]

No description provided.

[sebbel]

Please review @apinf/developers

[brylie]

While this works, it can be slightly more semantic. I.e. it can be more descriptive:

"If api documentation editor is enabled"

apiDocumentationEditorIsEnabled: function () {}

And then in the template:

{{# if apiDocumentationEditorIsEnabled }}
  ...
{{/ if }}

[sebbel]

Isn't this about whether the apidochost setting being set or not?

Am Dienstag, 26. Januar 2016 schrieb Brylie Christopher Oxley :

In client/layouts/master_layout/sidebar/sidebar.js
https://github.com/apinf/api-umbrella-dashboard/pull/794#discussion_r50813653
:

@@ -6,10 +6,15 @@ Template.sidebar.helpers({
var backendsCount = ApiBackends.find({managerIds: currentUserId}).count();
// return true if user has backends
return backendsCount > 0;

• },
• apiDoc_notEmpty : function() {

While this works, it can be slightly more semantic. I.e. it can be more
descriptive:

"If api documentation editor is configured"

apiDocumentationEditorIsEnabled: function () {}

And then in the template:

{{# if apiDocumentationEditorIsEnabled }}
...
{{/ if }}

—
Reply to this email directly or view it on GitHub
https://github.com/apinf/api-umbrella-dashboard/pull/794/files#r50813653
.

[brylie]

Yes, and the ApiDoc.host setting happens to enable the documentation editor, hence the verbosity.

[brylie]

In effect, the not_empty part, while accurate, does not describe the purpose of the check. Simply renaming the function can make the purpose more clear.

[brylie]

After a bit of thought, I have a suggestion to make our settings check more 'high-level' semantic and accurate to the underlying JavaScript.

1. Add a sub-field to the apiDocumentationEditor field, called enabled with a type of Boolean.
2. When the enabled field is true the host field is marked as required.
3. Then, in the apiDocumentationEditorIsEnabled template helper, simply return the value of the enabled field.
4. bonus points (optional) In the Settings autoform, use AutoForm {{ afFieldValueIs ... }} helper to selectively toggle the visibility of the host field, based on whether enabled is true

This is a pattern we can later repeat with other settings, such as integrations with Github, etc..

[sebbel]

This would also need a hook which would turn enabled on and off depending on the presence of apiDocumentationEditor.host

[brylie]

@sebbel, good point. Lets leave the hook for later, if necessary, and simply document the enabled field. That way, the host can be passed in and stored, even if the feature is not enabled (e.g. if the editor service is temporarily offline, but the admin wants to keep the host setting preserved.)

[sebbel]

Having Problems to grab the value of apiDocumentationEditor.enabled

[sebbel]

I have changed it according to your suggestions @brylie, but I am not really pleased with the outcome.

• The checkbox is not reactive, which means that the Documentation Editor Link only appears after the User has saved the settings.
• The Link appears even when there is no host set, I don't know if thats desired.

[brylie]

The save step is a good check to make sure the settings are correct before writing them to the collection.

[brylie]

The host setting should be conditionally required. You can tell AutoForm that "when the enabled field is true, the host field is required."

Here is a custom validation function you can add to the apiDocumentationEditor field schema:

"apiDocumentationEditor": {
  type: Object,
  optional: true
},
"apiDocumentationEditor.enabled": {
  type: Boolean,
  optional: true
}
"apiDocumentationEditor.host": {
  type: String,
  regEx: SimpleSchema.RegEx.Url,
  optional: true, // Optional must be true for custom validation
  custom: function () { // Custom validator logic
    // get the value of apiDocumentationEditor.enabled field
    let enabledFieldValue = this.field("apiDocumentationEditor.enabled").value;

    // if enabled is true, host field is required
    if (enabledFieldValue === true) {
      return "required"; // host field is required
    }
  }
}

[brylie]

Where is the subscription that provides data for this query?

We may need a new publication that returns only a single field from the Settings document, just as you are doing here. E.g.

Meteor.publish("singleSetting", function (setting) {
  // Set up a query settings object containing fields and a result limit
  let querySettings = {"fields": {}, "limit": 1};

  // Specify the setting field we want to retrieve (passed in as argument)
  querySettings.fields[setting] = 1;

  // Return a cursor containing only the requested setting from the Settings document
  let cursor = Settings.find({}, querySettings);

  return cursor;
});

Then, in your Template.created callback:

// Subscribe to the API Documentation Editor setting (including sub-fields)
instance.subscribe("singleSetting", "apiDocumentationEditor");

[sebbel]

Thanks for your feedback @brylie, Really like the generic publish method!

[brylie]

Check indentation here. Nested blocks should be one level (two spaces) indented.

[brylie]

This PR is nearly ready. There is just a minor issue with indentation (two spaces per indent level). Please be consistent with whitespace.

[sebbel]

Hope my edits helped maintain a clean whitespace.

[brylie]

Looks great! Thanks Sebastian :-)