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

WIP (docs driven development) - Preview API #1311

Closed
wants to merge 9 commits into from
Closed

Conversation

erquhart
Copy link
Contributor

@erquhart erquhart commented Apr 23, 2018

Alrighty folks, we're going to go docs driven for the preview API!

You do not have to be a maintainer to review this and help shape the API.

Related to #1041.

How to review:

  1. Read through the docs pages linked below.
  2. Create a review from the perspective of a developer looking to implement this API. Ask yourself:
    • Can we do this better?
    • Will this API result in a positive developer experience?
    • Is anything not covered by the API that should be?
  3. Resist the urge to focus on grammar, style, and typos. That comes way later.

I'll link live pages that are ready for review below. Feel free to go over files in this PR that are not linked below, just know that they're not considered ready for any actual review.

Let's get it 💪


Ready for Review

@verythorough
Copy link
Contributor

verythorough commented Apr 23, 2018

Deploy preview for netlify-cms-www ready!

Built with commit 95e64b3

https://deploy-preview-1311--netlify-cms-www.netlify.com

@verythorough
Copy link
Contributor

verythorough commented Apr 23, 2018

Deploy preview for cms-demo ready!

Built with commit 95e64b3

https://deploy-preview-1311--cms-demo.netlify.com

@erquhart erquhart force-pushed the api-docs-previews branch 4 times, most recently from 14238e7 to 5d13e68 Compare April 23, 2018 21:54
```html
<div>
<h1>Blog Post</h1>
A *brand new* post.
Copy link
Contributor

Choose a reason for hiding this comment

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

The description above has two asterisks (**brand new**) but there's only one here (*brand new*).

```

Registering a template may require other arguments depending on your template compiler. For more
information on `registerPreviewTemplate`, check out the [API docs](#), as well as the docs for your
Copy link
Contributor

Choose a reason for hiding this comment

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

Will we allow the preview template to be pulled in via a filename?

CMS.registerPreviewTemplate('blog', 'template.hbs');

If so, do we need to allow an option/switch for that so that we can easily differentiate between filenames and templates?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wasn't planning on it - the case where someone can actually use production templates in the CMS is pretty rare. It's most likely to be possible for projects using React components as templates, but in that case the file is going to be ES6/JSX, so the CMS still can't use it as is. We'd also have to grab templates via GitHub since they wouldn't be available in the deployed site.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Although this could also be used buy sites that do have custom templates for the CMS, but no build system for easily importing them in a registration script. This is worth considering.

return (
<div>
<h1>{fields.title.value}</h1>
<div>{collections.authors.fields}
Copy link
Contributor

Choose a reason for hiding this comment

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

This line is missing the </div>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch!

We'll likely change look and feel and improve presentation before this goes out - the big thing to review here is the new API itself, and whether it's adequate.

@erquhart
Copy link
Contributor Author

erquhart commented May 25, 2018

The big change so far:

Current
We provide an immutable map of field values, separate getter functions for specific bits like a renderable image URI or the field preview, and a general metadata object to collect secondary field data.

Proposed
Preview templates receive an object of fields, with nested fields directly in their parents. Each field that does not contain children has the values value, preview, and data.

What this means

  • getAsset is replaced by a src getter in a field's data property
  • fieldsMetaData is also replaced by each field's individual data property
  • widgetFor and widgetsFor are replaced by each field's individual preview property

Other benefits

  • Simple template languages like Mustache can be used with full Preview API functionality

Notes

  • A new guiding principle: the preview pane does a microscopic, transient version of what a static site generator does - converts some raw content and a template to HTML. The SSG only has raw values to work with. So when we provide "privileged" extras like widget previews and field data to the preview pane, they must only serve to bolster convenience and performance. For the most part, the preview template should have access to everything it needs to render with just raw values.
  • Whenever the question comes, "how do I get xyz in my preview template", our first response should be "how does the SSG get it?". Consider relations, for example, where the SSG only has a raw field value from the related entry.

(Developing)

@erquhart
Copy link
Contributor Author

Closing as stale, will re-open when ready.

@erquhart erquhart closed this Jul 31, 2018
@erquhart erquhart deleted the api-docs-previews branch July 31, 2018 21:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants