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.

Sign up for GitHub

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

Jump to bottom

WIP (docs driven development) - Preview API #1311

Closed
wants to merge 9 commits into from
Closed

WIP (docs driven development) - Preview API #1311

wants to merge 9 commits into from

Conversation

erquhart
Copy link
Contributor

@erquhart erquhart commented Apr 23, 2018 •
edited
Loading

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 •
edited
Loading

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 •
edited
Loading

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
Benaiah
Benaiah reviewed Apr 23, 2018
View reviewed changes
website/site/content/docs/custom-previews.md
```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*).

@erquhart erquhart force-pushed the api-docs-previews branch from 5d13e68 to 27010c8 Compare April 24, 2018 12:49
tech4him1
tech4him1 reviewed Apr 24, 2018
View reviewed changes
website/site/content/docs/custom-previews.md Outdated
```

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.

@erquhart erquhart mentioned this pull request Apr 24, 2018
Closed
10 tasks
@erquhart erquhart force-pushed the api-docs-previews branch from 02ea75b to 4585108 Compare April 27, 2018 15:13
@erquhart erquhart mentioned this pull request Apr 30, 2018
Open
@erquhart erquhart force-pushed the api-docs-previews branch from 04a4a71 to b9ecf1c Compare May 1, 2018 21:32
@verythorough verythorough mentioned this pull request May 6, 2018
Closed
@verythorough verythorough mentioned this pull request May 18, 2018
Merged
@erquhart erquhart force-pushed the api-docs-previews branch from f84b170 to f02774a Compare May 25, 2018 19:04
loremipson
loremipson reviewed May 25, 2018
View reviewed changes
website/site/content/docs/react-templates.md
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 •
edited
Loading

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
Reviewers

@loremipson loremipson loremipson left review comments

@Benaiah Benaiah Benaiah left review comments

@tech4him1 tech4him1 tech4him1 left review comments

Assignees
No one assigned
Labels
status: stale
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@erquhart @verythorough @loremipson @Benaiah @tech4him1