Skip to content

Conversation

@keegan-lillo
Copy link
Contributor

@keegan-lillo keegan-lillo commented Sep 7, 2020

Related to: #445

Summary

This PR allows the generated meta.path to be accessible in summary and preview_path when using nested collections.

eg:

collections:
  - name: 'pages'
    create: true
    label: 'Pages'
    label_singular: 'Page'
    folder: 'src/content/pages'
    preview_path: '{{dirname}}'
    media_folder: '/{{media_folder}}/pages/{{dirname}}'
    summary: '{{title}} - {{dirname}}'
    nested: { depth: 100 }
    meta: { path: { widget: string, label: 'Path', index_file: 'index' } }
    fields:
      - label: 'Title'
        name: 'title'
        widget: 'string'
      - label: 'Image'
        name: 'image'
        widget: 'image'
      - label: 'Body'
        name: 'body'
        widget: 'markdown'

Docs Preview

https://deploy-preview-4279--netlify-cms-www.netlify.app/docs/configuration-options/#preview_path
https://deploy-preview-4279--netlify-cms-www.netlify.app/docs/configuration-options/#summary
https://deploy-preview-4279--netlify-cms-www.netlify.app/docs/beta-features/#folder-collections-media-and-public-folder

Questions for reviewers

  • Is the naming convention of the PR title okay?
  • Thoughts on using nested_path as the template key? Going with dirname
  • Where should I document this? In the Beta Features or summary and preview_path sections?

Test plan

  1. Set up the config as above.
  2. Create a new "Nested Collection" with a Path set to dir-a/dir-b/hello-world and a Title of Hello World
  3. Save and publish
  4. Return to "Contents" list. The newly created item should look like this screenshot (Note the summary now includes the nested_path):
    Screen Shot 2020-09-08 at 9 14 16 am
  5. Click on the Hello World item to go into edit view
  6. Observe the destination for the "View Live" link. It should point to https://example.com/dir-a/dir-b/hello-world

Screen Shot 2020-09-08 at 9 15 10 am

To Do

  • Write tests
  • Update docs

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

My parents' dogs:
IMG_20200226_114708

@keegan-lillo keegan-lillo requested a review from a team September 7, 2020 23:23
const pathTemplate = collection.get('preview_path') as string;
let fields = entry.get('data') as Map<string, string>;
fields = addFileTemplateFields(entry.get('path'), fields);
fields = addNestedPath(entry.get('meta')?.get('path'), fields);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Typescript yelled at me if I used entry.getIn(['meta', 'path'])

identifier = '',
data = Map<string, unknown>(),
processor?: (value: string) => string,
processor?: (value: string, key: string) => string,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ideally it should be processor?: (key: string, value: string) => string, But I figured it was a less of a sweeping change to append the parameter instead.

@keegan-lillo keegan-lillo marked this pull request as draft September 8, 2020 04:12
@keegan-lillo keegan-lillo changed the title [WIP] feat(core) feat(lib-widget) Add {{nested_path}} to summary and preview_path feat(core) feat(lib-widget) Add {{nested_path}} to summary and preview_path Sep 8, 2020
@erezrokah erezrokah added the type: feature code contributing to the implementation of a feature and/or user facing functionality label Sep 8, 2020
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.

Thanks @keegan-lillo. The code changes look good, however we would like not to separate the terms path and nested_path - that is an internal CMS implementation.
The term meta is used to handle entry data that is not present in any field that is serialised to the output file.

Could we apply this for path in general and maybe use _path as a key to make is less of a breaking change if someone uses path as a field?

@keegan-lillo
Copy link
Contributor Author

@erezrokah thanks for the review! How about maybe meta_path or even meta.path? Do we use any other key with an underscore as a prefix?

Also, where would be the best place to document the change?

@erezrokah
Copy link
Contributor

@erezrokah thanks for the review! How about maybe meta_path or even meta.path? Do we use any other key with an underscore as a prefix?

We have a suggestion outlined here #3100, but it is a breaking change.

Since we have filename and extension it would make sense to have path (or something similar) that would work regardless if one uses nested collection (since the meaning is the same).
We can do it in a non breaking way by only adding it if it doesn't exist before passing to the string template processor.

For documentation, filename and extension are documented next to the relevant configuration item:
https://www.netlifycms.org/docs/configuration-options/#preview_path
https://www.netlifycms.org/docs/configuration-options/#summary

@keegan-lillo
Copy link
Contributor Author

Ah Yup. That makes sense. Okay so to clarify: let's just go with _path in preparation for #3100? Or should I do path with fall-back to _path if it is present in the fields?

@erezrokah
Copy link
Contributor

erezrokah commented Sep 13, 2020

Or should I do path with fall-back to _path if it is present in the fields?

Let's go with path and if it's present in in a field use the field.

@keegan-lillo
Copy link
Contributor Author

Let's go with path and if it's present in in a field use the field.

Isn't that the opposite of the current behaviour?

From: https://www.netlifycms.org/docs/configuration-options/#slug

The slug template can also reference a field value by name, eg. {{title}}. If a field name conflicts with a built in template tag name - for example, if you have a field named slug, and would like to reference that field via {{slug}}, you can do so by adding the explicit fields. prefix, eg. {{fields.slug}}.

Apologies for all the questions. Just want to be 100% clear.

@erezrokah
Copy link
Contributor

Isn't that the opposite of the current behaviour?

You're right, I didn't give enough thought. Doing so will prevent someone from using both the built in template and the field in the same template.

How about file_path? Less options for collisions and since it is only used for summary and preview path the impact won't be that big.

@keegan-lillo
Copy link
Contributor Author

Cool. That makes sense to me. Thanks for the feedback!

@keegan-lillo keegan-lillo changed the title feat(core) feat(lib-widget) Add {{nested_path}} to summary and preview_path feat(core) feat(lib-widget) Add {{file_path}} to summary and preview_path Sep 21, 2020
@keegan-lillo keegan-lillo force-pushed the core/nested-path-in-summary branch from 5071f87 to 1c6f31f Compare September 25, 2020 06:51
@keegan-lillo keegan-lillo changed the title feat(core) feat(lib-widget) Add {{file_path}} to summary and preview_path feat(core) feat(lib-widget) Add {{dirname}} to summary and preview_path Sep 25, 2020
@keegan-lillo keegan-lillo marked this pull request as ready for review September 25, 2020 06:52
@keegan-lillo
Copy link
Contributor Author

keegan-lillo commented Sep 25, 2020

@erezrokah This is now ready for review. I know we discussed using file_path, but after having another look, I felt it made more sense to include the changes as part of addFileTemplateFields(). I then realised, a more accurate key would be dirname as it doesn't actually include the file itself. Just the parent directory's path. It also then matches the node API too. Plus, I think dirname is maybe even less likely to have a naming collision with the fields.

label: loadedEntry.file.label,
author: loadedEntry.file.author,
updatedOn: loadedEntry.file.updatedOn,
meta: { path: prepareMetaPath(loadedEntry.file.path, collection) },
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 noticed this was missing in the previous implementation.

@erezrokah
Copy link
Contributor

@erezrokah This is now ready for review. I know we discussed using file_path, but after having another look, I felt it made more sense to include the changes as part of addFileTemplateFields(). I then realised, a more accurate key would be dirname as it doesn't actually include the file itself. Just the parent directory's path. It also then matches the node API too. Plus, I think dirname is maybe even less likely to have a naming collision with the fields.

Yes, this is much better!

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.

This is great @keegan-lillo.
Appreciate the discussion, tests and docs. Amazing work!

@erezrokah erezrokah merged commit 576e4f0 into decaporg:master Sep 28, 2020
vladdu pushed a commit to vladdu/netlify-cms that referenced this pull request Jan 26, 2021
@erezrokah erezrokah changed the title feat(core) feat(lib-widget) Add {{dirname}} to summary and preview_path feat(core): Add {{dirname}} to summary and preview_path Mar 1, 2022
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.

2 participants