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

feat(netlify-cms-widget-list): allow 'summary' field #3616

Merged
merged 6 commits into from
May 11, 2020

Conversation

d4rekanguok
Copy link
Contributor

@d4rekanguok d4rekanguok commented Apr 17, 2020

Hi folks! This PR is a WIP, in collaboration with @djMax

Summary

Fixes #3713

In the list widget's control, when an item is collapsed, only its type name is displayed. This makes it difficult for editors to understand the content of a list at a glance. This issue is more pronounced when editing a large list with many items of the same type.

image

This PR explores adding a summary field to each list's type, allow each item to display a summary based on its content, in the same fashion as a folder collection.

  - label: 'Typed List'
    name: 'typed_list'
    widget: 'list'
    types:
      - label: 'Type 1 Object'
        name: 'type_1_object'
+       summary: 'Type 1 | {{string}}'
        widget: 'object'
        fields:
          - { label: 'String', name: 'string', widget: 'string' }
          - { label: 'Boolean', name: 'boolean', widget: 'boolean' }
          - { label: 'Text', name: 'text', widget: 'text' }

image

We'd love to get feedback on this! We've also consider adding a summary field for the list widget itself that can be applied to all types.

Test plan

[TBA]

@d4rekanguok d4rekanguok requested a review from a team April 17, 2020 04:37
const fallbackLabel = itemType.get('label');
const summaryTemplate = itemType.get('summary');
return summaryTemplate
? compileStringTemplate(summaryTemplate, null, '', item)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We'll want to handle Date in an item's content here. Also we may want to pass in the type's label, so user can do i.e {{type.label}} | {{name}} instead of manually copying the label into the string template

Copy link
Contributor

Choose a reason for hiding this comment

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

You can add additional templates by modifying the item data, e.g.

item = item.set('label', itemType.get('label'))

Then {{label}} should work.
See: https://github.com/netlify/netlify-cms/blob/f88c83b97a18d9094240b688739d8b2909f19b0c/packages/netlify-cms-core/src/lib/formatters.ts#L125

Copy link
Contributor

Choose a reason for hiding this comment

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

If you have an existing label field you should be able to access it via fields.label

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @erezrokah, that's neat!

@hanneskuettner
Copy link
Contributor

Could we extend this to also allow a summary field on non-typed lists that only have field[s]? I can see this customizability be useful in that case as well. It would be located on the top level list definition in that case and apply to each entry individually.

@djMax
Copy link
Contributor

djMax commented Apr 17, 2020

Yeah, I think a "fallback" to the summary entry on the list declaration itself makes sense and covers all the bases.

@erezrokah erezrokah self-requested a review April 19, 2020 12:21
Copy link
Contributor

@erezrokah erezrokah left a comment

Choose a reason for hiding this comment

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

@d4rekanguok thanks for putting this up.
I'm good with this approach. Is there anything we can do on our side?

@d4rekanguok
Copy link
Contributor Author

d4rekanguok commented Apr 25, 2020

@erezrokah I don't think so, requiring compileStringTemplate from the core seems to work well! What do we need to do to move this PR forward? I have these in my todos:

  • make sure summary works well with date field in its list's item
  • check if fields.label has already worked, and/or pass label into item
  • put back the fields.get('name') fallback mentioned by @djMax

Perhaps adding summary to the list widget itself could be a second PR, I can see it making things a bit complicated (should we only use it for normal, 'un-typed' list? if not, how do we handle it in combination with each 'typed' list summary?)

I'm a bit occupied at the moment, would you be able to step in and continue this PR @djMax?

@erezrokah erezrokah added the type: feature code contributing to the implementation of a feature and/or user facing functionality label Apr 30, 2020
@erezrokah
Copy link
Contributor

erezrokah commented May 7, 2020

FYI, compileStringTemplate was moved to a dedicated package. See https://github.com/netlify/netlify-cms/blob/0ac3592e2617ef2c3b108d4256cd977787dcf4b2/packages/netlify-cms-widget-relation/src/RelationControl.js#L8 for an example.

let labelReturn = label
if (summary) {
const data = item.set('label', label)
labelReturn = stringTemplate.compileStringTemplate(summary, null, '', data)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@erezrokah It looks like in a string template, slug and date-related keys are hard-coded in:

https://github.com/netlify/netlify-cms/blob/0ac3592e2617ef2c3b108d4256cd977787dcf4b2/packages/netlify-cms-lib-widgets/src/stringTemplate.ts#L98-L104

When I tried using {{fields.label}}, it returns the same value as {{label}}. Should I pass in type's label under a different name/map?

Copy link
Contributor

Choose a reason for hiding this comment

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

The {{fields.*}} format is useful when you want to avoid collision with built in tags, for example let's say you want to reference a field named slug (see note here https://www.netlifycms.org/docs/configuration-options/#slug).
When referencing a field I would say it is safer to always use {{fields.fieldName}} to avoid possible future collisions (for example we recently added built in filename and extension template tags)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @erezrokah, thank you for further explaining about the field. format. I think I might be missing something, let me walk through this issue I have in my head:

Say I have an item Map with { label: 'One' }

The following code will replace label with item.getIn(['label']), returning 'Type: One'.

compileStringTemplate("Type: {{label}}", null, null, item) // "Type: One"

https://github.com/netlify/netlify-cms/blob/0ac3592e2617ef2c3b108d4256cd977787dcf4b2/packages/netlify-cms-lib-widgets/src/stringTemplate.ts#L106

I can override this label field by adding a new field to item, and now the following code should return Type: Two:

item = item.set('label', 'Two')
compileStringTemplate("Type: {{label}}", null, null, item) // "Type: Two"

This works!


Now here's the part I'm confused about. The following code should replace fields.label with the original label in item & return Type: One.

compileStringTemplate("Type: {{fields.label}}", null, null, item) // "Type: Two"

This doesn't work.

Looking at the code of getExplicitFieldReplacement which handles keys with fields. prefix, it removes fields. prefix and use data.getIn(keyToPathArray(fieldName)) to access the value of the label.

https://github.com/netlify/netlify-cms/blob/0ac3592e2617ef2c3b108d4256cd977787dcf4b2/packages/netlify-cms-lib-widgets/src/stringTemplate.ts#L69-L70

Looking at the code of compileStringTemplate, if a field is not a date-related key (year, month, day, etc), doesn't start with fields., or is not slug, it will access the value of the key via data.getIn(keyToPathArray(key), '').

https://github.com/netlify/netlify-cms/blob/0ac3592e2617ef2c3b108d4256cd977787dcf4b2/packages/netlify-cms-lib-widgets/src/stringTemplate.ts#L96-L107

Because of this, compileStringTemplate does exact the same for fields.label and label in stringTemplate:

  let data = Map({ label: 'One' })
  data = item.set('label', 'Two')
  labelReturn = stringTemplate.compileStringTemplate('{{fields.label}} | {{label}}', null, '', data)

I see no special handling for filename and extension either, so I'm lost on understanding how it works. Do I have to do something special so the original data is preserved? Thank you!

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry for not being clear enough. At the moment {{label}} {{fields.label}} have the same result, but we might want to have a built in label with special meaning in the future (like date and slug). If we do decide to make label a built in template var it would break behaviour for existing users that use {{label}} without the fields prefix and we would need to apply backwards compatibility code for that. To avoid doing so in the widget as well using fields.label is safer.
As for filename and extension, they are added when relevant here https://github.com/netlify/netlify-cms/blob/1c7ef1c457612242257d96af53a57c1460e45b31/packages/netlify-cms-core/src/lib/formatters.ts#L206
For example, adding those templates when evaluating the slug doesn't make any sense since the filename is based on the slug.

@jozsi
Copy link

jozsi commented May 9, 2020

I have a question/test case/bug report: would this work with the relation widget?
Right now, when a list is collapsed, it shows the valueField instead of displayFields:
image

I don't know if that is a bug or the new summary field would handle it - cause apparently, it only loads the displayFields value upon expanding the item, so it might not be available to summary while collapsed by default:
image

@erezrokah
Copy link
Contributor

Hi @jozsi, we recently added summary support to the relation widget (see example at the bottom of https://www.netlifycms.org/docs/widgets/#relation).
Make sure to update to the latest version.

@erezrokah
Copy link
Contributor

@d4rekanguok, I hope you don't mind, but I rebased the branch and added support for field and fields and some tests.
I'll update the docs and then merge.

@jozsi
Copy link

jozsi commented May 11, 2020

Dear @erezrokah,

I am on latest. If you are referring to the string template support, I tried it, no luck. But it might be related to #3739 so I am not going to pollute this issue further, sorry!

@erezrokah erezrokah merged commit 7cc4c89 into decaporg:master May 11, 2020
@d4rekanguok
Copy link
Contributor Author

@erezrokah Super neat, thank you for taking this over & taking it all the way (allowing summary field for regular lists) 👍!

Also thank you for the collaborator invitation, I'm honored to be a part of this project.

@d4rekanguok d4rekanguok deleted the feat/list-types-summary branch May 11, 2020 14:19
@erezrokah
Copy link
Contributor

Long overdue on the invitation 😄
Main advantage is that non forked PRs run faster (we use Cypress for e2e testing with parallel support, but that requires a secret token available only for non forked PRs)

@talves
Copy link
Collaborator

talves commented May 11, 2020

Was I removed as a maintainer @erezrokah @erquhart ?

@barthc
Copy link
Contributor

barthc commented May 11, 2020

A while ago maintainers without two-factor where removed, you might have been a victim.

@talves
Copy link
Collaborator

talves commented May 11, 2020

I have 2FA, that's not the case here.

@erquhart
Copy link
Contributor

Hey @talves - yes, we try to keep our access current. If you're thinking of getting involved again that would be awesome! I'll send your collaborator invite now.

@talves
Copy link
Collaborator

talves commented May 11, 2020

This really disappoints me after all the contributions I did. Although I have had limited time, there was no reason to remove my access, since I was and always have been a trusted maintainer. I am not surprised, but I think it warranted an email first. 😢

@erquhart
Copy link
Contributor

@talves that's a really good point, sending an email would have made a lot of sense. I reached out on Gitter a few times, but that definitely isn't the same thing. So sorry I didn't do a better job communicating here.

Please understand this in no way reflects a lack of respect for all of the work you've done personally - I cleared out all of our user access from contributors with more than 6 months of no activity. I'll certainly handle this kind of action better in the future.

@djMax
Copy link
Contributor

djMax commented May 11, 2020

Agreed, a big thank you from me too @d4rekanguok, as I didn't end up doing any real work on this one, but reap all the benefits!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: feature code contributing to the implementation of a feature and/or user facing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add summary option to list widget
8 participants