Skip to content
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

Registry: Encapsulate page layout (#1823) #1824

Closed
wants to merge 4 commits into from

Conversation

tedepstein
Copy link
Contributor

Partially addresses #1823. For now, focusing only on the Alternative Schema registry, but if we are OK with the approach, I can do the same for the other registries.

This extracts the boilerplate markdown to a new _includes/alternative-schema-entry.md file, referenced by an {% include %} tag in each collection item.

@tedepstein
Copy link
Contributor Author

tedepstein commented Feb 4, 2019

@MikeRalphson , please have a look when you get a moment.

BTW, you'll notice some changes to the YAML front matter. I renamed description to schemaTypeName and added SchemaTypeID so we don't have to rely on page.slug. I thought an explicit and descriptive property name would be a better choice.

If you think it's important to have the filename be the same as the schema type ID, we can add a YAML comment indicating that, and also include that requirement in the _contributing.md file.

@tedepstein
Copy link
Contributor Author

tedepstein commented Feb 4, 2019

@MikeRalphson, if you want to allow more flexibility for the contributor to include an extended description of the registry entry, I can think of a couple of ways to do this:

  1. Add a description property that can be multi-line, with markdown.
  2. Instead of having a single alternative-schema-entry.md, break this into alternative-schema-header.md and alternative-schema-footer.md.
    • The contributed registry entry has to include both of these.
    • The registry entry can, optionally, include descriptive content with markup between the header and footer.
    • The footer part might be empty for now, just a placeholder in case we need to add some boilerplate content after the contributed description.

The first option is more like OpenAPI's description properties, and also makes it formally part of the metadata (as YAML front matter). So I think it's a little cleaner. But the second option is OK too.

In either case, we can specify in our contributing guidelines that the description content should not use h1, or maybe should not use h1-h2 headings, so we reserve those for our own header and keep the content hierarchy more uniform.

@MikeRalphson
Copy link
Member

Generally 👍 to all of this, thanks!. The use of Jekyll includes reduces repetition and promotes more use of features Jekyll provides 'for free'.

That said, I am not in favour of replacing page.slug with a schemaTypeID for two reasons:

  1. the use of page.slug ensures that the markdown file has been named correctly, this is important because it is the markdown filename / slug which controls what URL the generated page ends up at. And the URLs in a registry should be correct, related to the entries and unchanging.
  2. It reduces consistency between registries if the identifier property is called schemaTypeID in one and extensionTypeID in a another. This makes iterating over all the registries much more complex / brittle.

I think splitting the description into a one-line summary and a description which can contain markdown will be a better move as it retains consistent property names across registries and echoes the OAS even more closely.

Copy link
Member

@MikeRalphson MikeRalphson left a comment

Choose a reason for hiding this comment

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

As per comments above.

@lornajane
Copy link
Contributor

Changes were requested some time ago, but the pull request hasn't been updated, so I'm closing. Feel free to open a new pull request, based off the current release, for us to review at any time.

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.

3 participants