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

Added "types" option to list widget #1857

Merged
merged 8 commits into from Dec 27, 2018

Conversation

Projects
None yet
@smnh
Copy link
Contributor

smnh commented Nov 6, 2018

update: I've changed the widget to types, and widget item property to type.

The list's "widgets" option allows defining lists of different item types.

Summary

The current list widget does not allow adding items of different types. Assume having a home page with different type sections - "carousel" and "spotlight". Each of these sections can have a different set of fields and both of these sections can be repeated multiple times:

home-sections

Following frontmatter example demonstrates a sections list with three sections (1. carousel, 2. spotlight, 3. carousel). Every section defines its widget type using the widget field.

---
title: Home
layout: home
sections:
  - widget: carousel
    header: Image Gallery 1
    template: carousel.html
    images:
      - images/image01.png
      - images/image02.png
      - images/image03.png
  - widget: spotlight
    header: Spotlight
    template: spotlight.html
    text: Hello World
  - widget: carousel
    header: Image Gallery 2
    template: carousel.html
    images:
      - images/image04.png
      - images/image05.png
      - images/image06.png
---

In order to allow such list to be rendered inside the CMS and to allow adding new items of different types, a new option for list widget called widgets was defined. The widgets option allows defining list of widgets a list could possible have (in other words, array of widgets).

Following example demonstrates how a list from the previous example could be defined:

- label: "Home Section"
  name: "sections"
  widget: "list"
  widgets:
    - label: "Carousel"
      name: "carousel"
      widget: object
      fields:
        - {label: Header, name: header, widget: string, default: "Image Gallery"}
        - {label: Template, name: template, widget: string, default: "carousel.html"}
        - label: Images
          name: images
          widget: list
          field: {label: Image, name: image, widget: image}
    - label: "Spotlight"
      name: "spotlight"
      widget: object
      fields:
        - {label: Header, name: header, widget: string, default: "Spotlight"}
        - {label: Template, name: template, widget: string, default: "spotlight.html"}
        - {label: Text, name: text, widget: text, default: "Hello World"}

Test plan

Using the above example for field configuration and data from page file the editor renders as following:

image

To the left of the "Add home section" button there is a <select> control that allows selecting one of the available widget types that were defined using list's widgets option.

Re-ordering, object creation/read/editing/deletion works as expected.

A picture of a cute animal (not mandatory but encouraged)

blacky

@verythorough

This comment has been minimized.

Copy link
Contributor

verythorough commented Nov 6, 2018

Deploy preview for netlify-cms-www ready!

Built with commit 3ed0efe

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

@erquhart erquhart referenced this pull request Nov 6, 2018

Closed

Modular Content Widget #1169

@erquhart

This comment has been minimized.

Copy link
Member

erquhart commented Nov 6, 2018

Awesome! There's prior art in #1066 and #1169, I commented on the latter to get some community eyes and hands on this for testing and feedback.

@TylerBarnes

This comment has been minimized.

Copy link

TylerBarnes commented Nov 6, 2018

This looks awesome! Can't wait to try it out

@cjbrooks12

This comment has been minimized.

Copy link

cjbrooks12 commented Nov 6, 2018

Awesome, I can't wait to play around with it and see how it all works!

My one suggestion is to be able to customize the key used to identify the "widget type". To be a general-purpose modular widget list, it needs to not have hard-coded keys being read from the Front Matter since different SSGs might have a similar concept of modular lists but read the config differently.

Basically, there should be an option in the list item config to allow widget to be changed as the developer or SSG requires:

---
title: Home
layout: home
sections:
  - widget: carousel # <-- this key should be configurable, not always `widget`
    header: Image Gallery 1
    template: carousel.html
    images:
      - images/image01.png
      - images/image02.png
      - images/image03.png
  - widget: spotlight # <-- this key should be configurable, not always `widget`
    header: Spotlight
    template: spotlight.html
    text: Hello World
  - widget: carousel # <-- this key should be configurable, not always `widget`
    header: Image Gallery 2
    template: carousel.html
    images:
      - images/image04.png
      - images/image05.png
      - images/image06.png
---

Here's my comment on the other, older PR for reference: #1169 (comment)

@criticalmash

This comment has been minimized.

Copy link

criticalmash commented Nov 6, 2018

I think this is a very elegant solution for Modular Content, thanks @smnh.

I checked out the tree and tried it out using the "Home Section" sample field. Here's what I saw:

The template fields for each widget where not hidden. I'd expect these to be hidden when used in a production environment (using widget: 'hidden'). I can understand if you were displaying them for demonstration purposes. (it also might be nice to offer a select list of widgets in case the developer needs to provide a choice of templates).

The widget fields were also missing their configured default values.

I also saw variations of the following error message in the browser console:

Warning: Failed prop type: The prop `files` is marked as required in `MediaLibrary`, but its value is `undefined`.

But I also seen it when testing the master branch, so it might not have anything to do with this pull.

Finally, I'm wondering how I can review the YAML that's outputted when I save or publish a page.

Looking forward to testing this some more.

@smnh

This comment has been minimized.

Copy link
Contributor Author

smnh commented Nov 7, 2018

@cjbrooks12, yes a widgetKey (like you have demonstrated in #1169 (comment)) or a widgetField is a great idea.

Here, an example with widgetField that defines the name of the field that will be added to every item in the list to define its type. The default one could be widget so if it is not specified, widget will be used.

- label: "Home Section"
  name: "sections"
  widget: "list"
  widgets:
    - label: "Carousel"
      name: "carousel"
      widget: object
      widgetField: widget
      fields:
        ...
    - label: "Spotlight"
      name: "spotlight"
      widget: object
      widgetField: widget
      fields:
        ...

What do you think regarding the name of the widgets field, does it make its intention clear? I saw that in the other PR the types have been used instead.

@criticalmash, yes the template field was not hidden only for demonstration purposes and as a consequence of me writing that example too quickly. But in any case, the fields of a specific widget/type can be any arbitrary fields. That was just an example.

Regarding default values, yes, I've noticed that too, it seems that default values are not working at all. Even when you use production netlify-cms from unpkg.com.

As for the preview, you are right, I didn't handle that. I will make the appropriate changes to include widget values in preview.

@shaunbent

This comment has been minimized.

Copy link

shaunbent commented Nov 7, 2018

@smnh Amazing work on this. 😍❤️

It's great that you can introduce such a level of flexibility to NetlifyCMS with what seems to be a relatively small change.

What are your thoughts about the UI around adding a new section? I wonder if the two elements is a little confusing. Maybe it could be combined into a single element similar to what was proposed in #1169

@smnh

This comment has been minimized.

Copy link
Contributor Author

smnh commented Nov 7, 2018

Hi @shaunbent,

Yes, I was thinking about making it a single element. But I've tried that Dropdown component that was used in #1169 but for some reason it didn't work. We can always change this UI later thought.

@smnh

This comment has been minimized.

Copy link
Contributor Author

smnh commented Nov 7, 2018

As @cjbrooks12 suggested, I've added a typeKey that can be used to define the type field that will be set on list items. The default one is type. I've also changed the field name from widgets to types.

As @criticalmash suggested I've added a way to preview the list and object properties in a much convenient matter by using nested lists. The list and object previews work for all kind of lists, not only one with types.

Here is a full example for the configuration, editor, preview pane and final frontmatter result:

collections:
  - name: Page
    label: page
    create: true
    extension: md
    slug: '{{slug}}'
    fields:
      - label: "Title"
        name: "title"
        type: "string"
        default: "Hello World"
      - label: "Home Section"
        name: "sections"
        widget: "list"
        types:
          - label: "Carousel"
            name: "carousel"
            widget: object
            fields:
              - {label: Header, name: header, widget: string, default: "Image Gallery"}
              - {label: Template, name: template, widget: string, default: "carousel.html"}
              - label: Images
                name: images
                widget: list
                field: {label: Image, name: image, widget: image}
          - label: "Spotlight"
            name: "spotlight"
            widget: object
            fields:
              - {label: Header, name: header, widget: string, default: "Spotlight"}
              - {label: Template, name: template, widget: string, default: "spotlight.html"}
              - {label: Text, name: text, widget: text, default: "Hello World"}
---
title: Hello World
sections:
  - header: This is header
    images: []
    template: This is template
    type: carousel
  - header: This is Spotlight Section
    template: Foo bar
    text: Hello World
    type: spotlight
---

image

Following example demonstrates the preview of objects and lists with simple fields:

image

The configuration for these additional fields is:

- label: Simple list
  name: simple_list
  widget: list
  fields:
    - {label: Foo, name: foo, widget: string, default: "something"}
    - {label: Bar, name: bar, widget: number, default: 3}
    - {label: Baz, name: baz, widget: date}
- label: Just an object
  name: my_object
  widget: object
  fields:
    - {label: One, name: one, widget: string, default: "Spotlight"}
    - {label: Two, name: two, widget: number, default: 4}
    - {label: Three, name: three, widget: date}

And the stored values are:

simple_list:
  - baz: 2018-11-07T23:37:15.088Z
    foo: My Value
my_object:
  one: Value 1
  three: 2018-11-08T23:37:52.059Z
  two: '5'

@smnh smnh changed the title Added "widgets" option to list widget Added "types" option to list widget Nov 7, 2018

@verythorough

This comment has been minimized.

Copy link
Contributor

verythorough commented Nov 8, 2018

Deploy preview for cms-demo ready!

Built with commit 3ed0efe

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

@cjbrooks12

This comment has been minimized.

Copy link

cjbrooks12 commented Nov 9, 2018

I've been playing around with it and it's working great, this is gonna be so nice! The core functionality works perfectly, customizing the typeKey works great, thanks for finally putting this together!

The only issue I've found is that if a list item in the Front Matter doesn't have a type property, it blows up and nothing displays instead of failing gracefully with a helpful error message.

Here's my stacktrace, though it doesn't seem too helpful...

TypeError: Cannot read property 'get' of undefined
    at ProxyComponent.objectLabel (netlify-cms.js:235024)
    at ProxyComponent.objectLabel (netlify-cms.js:90450)
    at ListControl._defineProperty (netlify-cms.js:234968)
    at netlify-cms.js:31978
    at List.__iterate (netlify-cms.js:31168)
    at IndexedIterable.mappedSequence.__iterateUncached (netlify-cms.js:31977)
    at seqIterate (netlify-cms.js:29566)
    at IndexedIterable.IndexedSeq.__iterate (netlify-cms.js:29282)
    at IndexedIterable.toArray (netlify-cms.js:33220)
    at List [as constructor] (netlify-cms.js:31027)
@smnh

This comment has been minimized.

Copy link
Contributor Author

smnh commented Nov 9, 2018

@cjbrooks12, yeah, you are right.
Better to have some kind of error message than just crashing everything.
I will add support for that.

@gil-- gil-- referenced this pull request Nov 9, 2018

Closed

RFP: Admin Headless CMS #44

@smnh

This comment has been minimized.

Copy link
Contributor Author

smnh commented Nov 10, 2018

@cjbrooks12, I've added error messages for typed lists as you have suggested.

The error message is shown both on the control and preview panes. If type (or custom typeKey) property is missing, it shows the "has no 'type' property" error message, otherwise, if it is set to a non existing type, it shows the "has illegal 'type' property".

Check this out:
image

I've also added typed lists to the kitchen sink.

@cjbrooks12

This comment has been minimized.

Copy link

cjbrooks12 commented Nov 10, 2018

Update is looking great, I've been playing around with it all morning and can't find any more issues. Great job, I can't wait until this gets merged!

@shaunbent

This comment has been minimized.

Copy link

shaunbent commented Nov 12, 2018

Thank you so much for pushing this @smnh - awesome work.

Whats your thoughts on the possibility of having this merged @erquhart?

@tb

This comment has been minimized.

Copy link

tb commented Nov 21, 2018

@erquhart since Mach (#1169) this feature can not make it into master :(

@liquidvisual

This comment has been minimized.

Copy link

liquidvisual commented Nov 21, 2018

Any update on this feature?

@erquhart

This comment has been minimized.

Copy link
Member

erquhart commented Nov 21, 2018

Reviewing soon 👍🏼

Sent with GitHawk

@erquhart

This comment has been minimized.

Copy link
Member

erquhart commented Nov 27, 2018

Great work on this @smnh, definitely want to see this through to merge.

A few takeaways from my first pass:

  1. Ideally, changes would be entirely contained within the list widget package (and object top bar), eg. no new utils.
  2. To help with number 1, I think we can drop the preview pane changes. They're the biggest contributor of noise in this PR, and they aren't a critical need. Maybe a separate PR for that.
  3. We should make the effort to use the single dropdown UI.

Taking another pass between today and tomorrow, may add a commit as well, but if you want to tackle these points before that, I'll get your changes reviewed when they come through.

@tb don't worry, we'll get this one merged 💪 Thanks again @smnh!

@smnh

This comment has been minimized.

Copy link
Contributor Author

smnh commented Nov 27, 2018

Thanks for taking a time to review this @erquhart
I can fix all these on Friday and let you know when I am done. Otherwise, if you have the time, you can make the changes as well.

@erquhart

This comment has been minimized.

Copy link
Member

erquhart commented Nov 27, 2018

Sounds good 👍🏼👍🏼

Sent with GitHawk

@smnh smnh force-pushed the stackbithq:mixed-list branch from 1db2f5a to 190f606 Dec 1, 2018

@smnh

This comment has been minimized.

Copy link
Contributor Author

smnh commented Dec 2, 2018

@erquhart, as you have asked I've moved the utility functions from netlify-cms-lib-util.js to a file under netlify-cms-widget-list package (later these functions might be used inside EditorPreviewPane.js), reverted changes to EditorPreviewPane.js and replaced select control and a button with a single dropdown list.

image

@erquhart

This comment has been minimized.

Copy link
Member

erquhart commented Dec 4, 2018

Awesome work man 👍

I think we should drop the label change for the object widget, folks can add a custom preview if they need that.

Question: what happens if one or more type definitions is a widget other than object? Like a string widget that isn't nested in anything?

@smnh

This comment has been minimized.

Copy link
Contributor Author

smnh commented Dec 4, 2018

@erquhart

I think we should drop the label change for the object widget, folks can add a custom preview if they need that.

You mean this one in ObjectPreview.js?

<div>{field.get('label', field.get('name'))}</div>

Question: what happens if one or more type definitions is a widget other than object? Like a string widget that isn't nested in anything?

Basically, because typed list must know its types when it is parsing the items, every item must be an object with type property. For example given the next list, there is no way to know what type every value is (except the booleans and number):

my_list:
  - true  # this is boolean, easy to guess
  - 4  # this is int number, also easy
  - hello world  # is this a text, string or maybe a markdown?
  - my_image.jpg # even more complicated. looks like an image or a file, but also can be a simple text or string

Therefore, every item in such list must have a type and a value properties (actually, value is just an arbitrary name and can be anything):

my_list:
  - type: boolean_type
    value: true
  - type: int_number_type # in the type definition it will be a "widget: number" with "valueType: int"
    value: 4
  - type: string_type
    value: Hello World
  - type: image_type
    value: my_image.jpg # even more complicated. looks like an image or a file, but also can be a simple text or string

To make it work, every item should be a widget of type object with a single field, for example value:

- label: "My List"
  name: "my_list"
  widget: "list"
  types:
    - label: "String"
      name: "string_type"
      widget: object
      fields:
        - {label: Value, name: value, widget: string, default: "Hello World"}
    - label: "Number"
      name: "number_type"
      widget: object
      fields:
        - {label: Value, name: value, widget: number, valueType: int, default: 10}
    ...

Later, it can be made even simpler. If one of the types is not an object widget, then an object with two fields type and value will be automatically inferred. And if one wants different keys then he will be able to use typeKey and valueKey to change the default key names.

So, one day it can be like this:

- label: "My List"
  name: "my_list"
  widget: "list"
  types:
    - label: "String"
      name: "string_type"
      widget: string
      default: "Hello World"
    - label: "Number"
      name: "number_type"
      widget: number
      valueType: int
      default: 10
    ...

That is why I left widget even though currently only object is supported.

@erquhart

This comment has been minimized.

Copy link
Member

erquhart commented Dec 4, 2018

Yes, the ObjectPreview.js changes should be dropped.

Because custom widgets can be registered, I don't expect we'll ever be able to infer types with complete accuracy. I'm not suggesting that we allow widgets other than object, I'm suggesting that we drop the widget key and just use the object widget internally. The list widget control already imports it.

@watzing

This comment has been minimized.

Copy link

watzing commented Dec 11, 2018

Any update on this?

@erquhart

This comment has been minimized.

Copy link
Member

erquhart commented Dec 17, 2018

I'm suggesting we continue using the object widget under the hood as we already do in the list widget:

<ObjectControl
classNameWrapper={cx(classNameWrapper, { [styles.collapsedObjectControl]: collapsed })}
value={item}
field={field}
onChangeObject={this.handleChangeFor(index)}
editorControl={editorControl}
resolveWidget={resolveWidget}
forList
/>

Unless you use field instead of fields, the list widget is actually a list of object widgets, and the fields are passed to the object widget control component. We should do the same with types.

@smnh

This comment has been minimized.

Copy link
Contributor Author

smnh commented Dec 18, 2018

@erquhart, indeed, both fields and types lists are lists of objects. But as opposed to the fields list, where each object has exactly the same fields, the types list must be defined in a way that allows specifying a label, and more importantly the name for each object type, This name field serves as the type identifier for objects inside that list. Such that, every object in a list must have the type field that points back to the name of the object and by so specifying its type. Therefore, the definition of such a list must have items with name and fields properties (the widget property is optional, and if not specified it will still work):

config.yml:

...
- label: "Home Sections"
  name: "sections"
  widget: "list"
  types:
    - label: "Carousel"
      name: "carousel"
      widget: object    # this is optional, if widget is not specified it will still work
      fields:
        - {label: Header, name: header, widget: string, default: "Image Gallery"}
        - {label: Template, name: template, widget: string, default: "carousel.html"}
        - label: Images
          name: images
          widget: list
          field: {label: Image, name: image, widget: image}
    - label: "Spotlight"
      name: "spotlight"
      fields:
        - {label: Header, name: header, widget: string, default: "Spotlight"}
        - {label: Template, name: template, widget: string, default: "spotlight.html"}
        - {label: Text, name: text, widget: text, default: "Hello World"}

index.md:

---
title: Home
layout: home
sections:
  - type: carousel
    header: Image Gallery 1
    template: carousel.html
    images:
      - images/image01.png
      - images/image02.png
      - images/image03.png
  - type: spotlight
    header: Spotlight
    template: spotlight.html
    text: Hello World
  - type: carousel
    header: Image Gallery 2
    template: carousel.html
    images:
      - images/image04.png
      - images/image05.png
      - images/image06.png
---

P.S.:
Under the hood, the types list still uses these lines with ObjectControl you've mentioned - and the field value is computed based on the type field of the item and the name of one of the defined objects:

field = getTypedFieldForValue(field, item);

@erquhart

This comment has been minimized.

Copy link
Member

erquhart commented Dec 19, 2018

Cool - I think we're on the same page, my only concern is folks using non-object widgets and getting unexpected results. As long as we make things clear in the docs, this should be fine.

@liquidvisual

This comment has been minimized.

Copy link

liquidvisual commented Dec 19, 2018

Would love if we could get this awesome new feature before Xmas. It'd be great to play with over the holidays :)

@watzing

This comment has been minimized.

Copy link

watzing commented Dec 19, 2018

@liquidvisual It would be a christmas miracle

smnh added some commits Nov 6, 2018

Added "widgets" option to list widget
The "widgets" option allows defining lists of different item types.
More work on list with types
- changed widgets field to types
- added typeKey property
- added list preview for lists and objects
Added error handling for typed lists
- Added typed list example to kitchen sink
Typed list changes
- removed utils from netlify-cms-lib-util
- removed changes to EditorPreviewPane.js
- replaced select with button with dropdown list

@smnh smnh force-pushed the stackbithq:mixed-list branch from e0cb84d to 4d945b0 Dec 20, 2018

@smnh

This comment has been minimized.

Copy link
Contributor Author

smnh commented Dec 20, 2018

I've clarified in list widget documentation, that typed lists can only include widgets of type object.

@erquhart

This comment has been minimized.

Copy link
Member

erquhart commented Dec 20, 2018

@smnh looking at the demo, typed list values aren't showing in the preview pane, any idea why?

@smnh

This comment has been minimized.

Copy link
Contributor Author

smnh commented Dec 20, 2018

Yeah, because by default preview pane does not show objects properties nor lists values.
I've changed "preview pane" to show all these values, but you have asked to remove it from this PR:

  1. To help with number 1, I think we can drop the preview pane changes. They're the biggest contributor of noise in this PR, and they aren't a critical need. Maybe a separate PR for that.
@erquhart

This comment has been minimized.

Copy link
Member

erquhart commented Dec 20, 2018

Not sure what you mean, object and list properties do show in the preview pane, just try editing any object or list in the kitchen sink. As far as I understood, those changes you mentioned were adding formatting, not displaying values that were otherwise hidden.

@smnh

This comment has been minimized.

Copy link
Contributor Author

smnh commented Dec 20, 2018

Ok, some clarification.

So there were two different issues:

First, regarding object and array properties. When I've wrote that preview pane does not show properties ("indexes" in the case of arrays), I've meant that it does not show the property names (or the field names) of the respective values. Although it does show the labels and values of object fields. So if one have a nested object, it does not show to which nested properties and nested objects a value belongs, it only shows the label and the value. In case of the typed array, it becomes more difficult to recognize to which specific item type a value may belong. So to make it clear, I've added lists with property names (for objects) and indexes (for arrays) to the preview. And in case of typed arrays, the type of each item was shown next to its index:

typed list preview example

Naturally, this change was made inside EditorPreviewPane.js.

Second, because typed array can have different object types, it was necessary to infer the correct object type of each item by its type and list's types. And only then, fields and values could be passed further to different preview methods.

This change also naturally went into EditorPreviewPane.js.

Then from your comments I came to a conclusion that all changes to this file should be removed. What I eventually did. I understand now that some of these changes should have stayed.

These changes are still available in the commit history. I will try to find some time in the next week or two to refactor it and pull out only what is needed for what I've explained in my the second point. If someone have the time, you are welcome.

@erquhart

This comment has been minimized.

Copy link
Member

erquhart commented Dec 20, 2018

Understood - I do disagree on the placement of code, though. I could be wrong, but I'm not certain the concept of "types" needs to be a concern in the CMS core. I'd expect the list widget that implements types to parse said types in its own preview component.

Regarding the unfortunately lengthy lifespan of this PR, I want to say how seriously thankful I am for all of the effort that has gone into this! The maintainers of this project don't take contributions lightly, neither the time they represent nor the blocking effect a longstanding PR can have on downstream projects. If you check the repo history, we've been pushing hard to get PR's through with minimal fuss, and increasingly so in the past few months. This PR carries a considerable surface for potential bugs both in code and UX, and represents a shift in the kinds of editing that typically can occur in Netlify CMS, hence the delay.

I don't believe my latest change request is superficial, though. We're not talking trivial bits like coding style. Architecturally, it's very important to keep extensions separate from the CMS core. We weren't diligent about this in the beginning and the effort to right the ship was considerable. We as a community, and especially the maintainers, are also responsible to all of the CMS users that will be impacted by any bugs that arise from this feature, impact that will occur whether they want the feature or not. Our lack of tests is regrettable, and contributes heavily to this risk factor.

I'll look into pulling down the latest work here and getting previews working so we can merge before Christmas (challenge accepted @watzing 😉). Again thank you @smnh et al for the work, review, and overall push to get this out 💪

@erquhart

This comment has been minimized.

Copy link
Member

erquhart commented Dec 22, 2018

Update:

Spent time on this today, and I think the CMS isn't ready for it (architecturally). But you all clearly are 😄. I don't disagree with the aim of the PR at all, I'm more in disagreement with how we're currently processing widgets and values in the editor, and this very awesome feature puts a lot of stress on that exact point.

I'm going on break for three weeks starting today, but I don't want to hold this up any longer. I also don't want to roll back to the commit that added unordered list tags to our core. So I propose we merge this as is as a beta feature. We've done this in the past, it puts a disclaimer on the whole thing so we can still change the introduced API without a major release, and there's no guarantee it won't break during usage.

That also means previews won't work, so we'll be pending a PR to get that going. But at least you'll have the functionality, and I'll keep an eye out in case a previews PR comes through so it doesn't have to wait longer than necessary.

Looking for feedback here. Yay? Nay?

@liquidvisual

This comment has been minimized.

Copy link

liquidvisual commented Dec 22, 2018

A huge YAY. Thanks for squeezing this in, Shawn! Also massive thanks to @smnh for all your hard work on this, you're a legend.

@tomrutgers

This comment has been minimized.

Copy link
Collaborator

tomrutgers commented Dec 22, 2018

Yay! As long as it doesn’t break existing functionality. It would be awesome if people can share their experiences on Gitter so we can figure out how to troubleshoot it. I think a beta release is an excellent idea, if we’re confident enough that we can eventually release it as a finished feature. Great work guys!

@smnh

This comment has been minimized.

Copy link
Contributor Author

smnh commented Dec 22, 2018

I agree. If you will merge it now, then later I will create another PR with previews for the typed list values.

@erquhart, regarding your comment where the list preview code should live. I agree that it should be inside ListPreview and not inside EditorPreviewPane.js which is inside cms-core. But the thing is, that there is no actual ListPreview, instead, list widget actually uses ObjectPreview for its preview:

export { ObjectPreview as ListPreview } from 'netlify-cms-widget-object';

And the ObjectPreview itself does not have any specific logic as well. So all the code responsible for showing nested object properties/array indexes is again inside EditorPreviewPane.js.

I didn't want to change all that because first, it would be a much bigger change, and second, it might break something because I am not aware of the reasons why it was written this way from the beginning.

If you ask me, I think "EditorPreviewPane.js" should be refactored and all the logic specific to different widgets should go to preview components of each widget.

@tomrutgers, I took care to make sure that all changes are specific to a case when you add types property to a list widget. So it should not break existing lists.

@erquhart

This comment has been minimized.

Copy link
Member

erquhart commented Dec 26, 2018

@smnh agreed, that's what I was referring to about our handling of widget values, needs an overhaul.

I'll get this merged and released as a beta feature very soon, should be able to release tomorrow.

Sent with GitHawk

@erquhart erquhart merged commit 8ddc168 into netlify:master Dec 27, 2018

2 checks passed

netlify/cms-demo/deploy-preview Deploy preview ready!
Details
netlify/netlify-cms-www/deploy-preview Deploy preview ready!
Details
@erquhart

This comment has been minimized.

Copy link
Member

erquhart commented Dec 27, 2018

Released in 2.3.2.

Merry Christmas :)

@watzing

This comment has been minimized.

Copy link

watzing commented Dec 27, 2018

Thanks Santa @erquhart , looking forward to having a play with my new toy

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment