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: multi content authoring #4139

Merged
merged 67 commits into from
Sep 20, 2020
Merged

Conversation

erezrokah
Copy link
Contributor

@erezrokah erezrokah commented Aug 11, 2020

Follow up on #3366
Fixes #716
Also fixes #4122

Example configuration:

# can be overridden at the collection level
i18n: { structure: multiple_folder, locales: ['en', 'de', 'fr'] }
collections:
  - name: locale_folders
    label: Locale Folders
    folder: content/locale_folders
    create: true
    i18n: true
    fields:
      - label: Title
        name: title
        widget: string
        # same as 'i18n: translate'
        i18n: true
      - label: Date
        name: date
        widget: datetime
        # this field will be copied as is
        i18n: 'duplicate'
      - label: Body
        name: body
        # this field will be omitted
        widget: markdown

TODO

Thanks

@barthc, @joallard and a lot more I can't remember at the moment

@github-actions github-actions bot added the type: feature code contributing to the implementation of a feature and/or user facing functionality label Aug 11, 2020
@barthc
Copy link
Contributor

barthc commented Aug 12, 2020

Thanks, @erezrokah, some comments.

  1. The locale dropdown seems off.
    i18n-editor-1

I would suggest something like so:
i18n-editor-2

  1. The current locale on the left editor is now validated instead of the default locale.

  2. The duplicate and hidden widgets don't work for nested fields.

  3. Using the below gif, the title fr is saved under fr locale while body fr is saved under default locale. Only happens for markdown widget.
    Peek 2020-08-12 22-12

@erezrokah erezrokah force-pushed the feat/multi-content-authoring-2 branch 4 times, most recently from 544d2d0 to 23e7ac8 Compare August 16, 2020 14:29
@erezrokah erezrokah marked this pull request as ready for review August 16, 2020 15:05
@erezrokah erezrokah requested a review from a team August 16, 2020 15:05
@@ -136,6 +140,7 @@ export const defaultSchema = ({ path = requiredString } = {}) => {
status: requiredString,
}).required(),
})
.xor('entry', 'dataFiles')
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Either entry (for backwards compatibility) or dataFiles must be included in the request, but not both

try {
release = await mutex.acquire();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This fixes #4122

@@ -231,11 +242,23 @@ export const localGitMiddleware = ({ repoPath, logger }: GitOptions) => {
const diffs = await getDiffs(git, branch, cmsBranch);
const label = await git.raw(['config', branchDescription(cmsBranch)]);
const status = label && labelToStatus(label.trim());
const updatedAt =
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixes an unrelated bug of dates not showing for editorial workflow entries in local git mode

@@ -6,4 +6,5 @@ export default {
minimize_collapsed: { type: 'boolean' },
label_singular: { type: 'string' },
},
prohibited: ['i18n'],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Don't allow a list field to have a i18n configuration

Copy link

@reimertz reimertz Aug 18, 2020

Choose a reason for hiding this comment

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

This is a pretty big change from the previous pull-request.
What was the reason for not including this into the PR and what can I do to help add/implement this again?

The reason why I ask is because I've spent this summer to implement internationalization and used the previous fork and had everything working and just introduced the entire team to my code and it relied on lists. 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, going to fix this soon 😄

Choose a reason for hiding this comment

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

Wow, that was a fast answer!

I feel like an a**hole for asking but do you have an idea what soon means?

  • before this PR is merged?
  • days
  • weeks
  • I don't know

The reason why I ask is because I'm trying to set expectations for my team.
Of course, if you cannot answer or do not want to answer, I totally get it. <3

Choose a reason for hiding this comment

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

And again, point me in an direction and I'll try and help out!

Choose a reason for hiding this comment

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

What I'll do then is to fork this PR and fix so that multicontent works with single files again and create a new PR once this one has landed. So excited!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Before the PR is merged 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The issue with lists is when using i18n: duplicate:
image

List widgets has a lot of internal state (e.g. toggle status).
To make that work in a good way for i18n: duplicate we would need to synchronize the internal state of widgets across editors which would require too much work at the moment.
The initial thought was to disable it completely, however it is better to at least allow i18n: true.

import ajvErrors from 'ajv-errors';
import { formatExtensions, frontmatterFormats, extensionFormatters } from 'Formats/formats';
import { getWidgets } from 'Lib/registry';
import { I18N_STRUCTURE, I18N_FIELD } from '../lib/i18n';

const localeType = { type: 'string', minLength: 2, maxLength: 10, pattern: '^[a-zA-Z-_]+$' };
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if there is a error at UI when the locales string is not valid.
But it would be great if there a document to explain the limitation of the locale string.
Because some people like me, may not take a look at the code.

Example:
Limitation Locale Strings

  • Minimum length 2
  • Maximum length 10
  • Allow characters a-z,A-Z and "-", "_"

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 use https://ajv.js.org/ for validation so a relevant error is printed.
An example error:
image

@erezrokah erezrokah force-pushed the feat/multi-content-authoring-2 branch from d0d32ea to 1380884 Compare August 17, 2020 13:15
@barthc
Copy link
Contributor

barthc commented Aug 17, 2020

@erezrokah final thoughts. The markdown widget issue still lingers. The preview only works for default locale and widget serialization as well https://github.com/netlify/netlify-cms/blob/master/packages/netlify-cms-core/src/lib/serializeEntryValues.js. I think storing all locale values under ['data'] would clean up codes and will allow switching the default locale with breaking code.

@erezrokah
Copy link
Contributor Author

The markdown widget issue still lingers

Thanks @barthc, I couldn't reproduce this issue. Can you post a more detailed reproduction?

The preview only works for default locale and widget serialization as well https://github.com/netlify/netlify-cms/blob/master/packages/netlify-cms-core/src/lib/serializeEntryValues.js.

Good point, let me look into that.

I wanted duplicate fields to show up in the UI (hence I removed the code that duplicates them in serializeEntryValues).
Will make sure that works as expected.

allow switching the default locale with breaking code.

Not sure I understand this point.

@barthc
Copy link
Contributor

barthc commented Aug 17, 2020

Not sure I understand this point.

For existing entry, let's say a collection has locales as [ 'en', 'de'] switching the default locale to a new locale ['fr', 'en', 'de'], the code breaks here first https://github.com/netlify/netlify-cms/pull/4139/files#diff-57dc49557fe997703a84eff0dc845cf3R221 not sure if it does in other places.

I couldn't reproduce this issue. Can you post a more detailed reproduction?

Peek 2020-08-17 17-25

As I said it's markdown widget issue, a simple fix would be to add the current locale check for widget.js shouldComponentUpdate function.

@rchrdnsh
Copy link

This is really exciting! XD

@erezrokah
Copy link
Contributor Author

widget serialization

I think this is partially broken at the moment - couldn't find any call to deserializeValues

@erezrokah erezrokah force-pushed the feat/multi-content-authoring-2 branch from 33434d5 to ccde073 Compare August 18, 2020 18:37
@erezrokah erezrokah force-pushed the feat/multi-content-authoring-2 branch from ccde073 to 1621ec9 Compare September 6, 2020 19:32
@reimertz
Copy link

Hello there and hope everybodys summer was great. ☀️

First, thank you all for moving this track forward and thanks @erezrokah for adding support for list widgets. it was a really nice gesture.

We at 29k.org are about to move forward and migrate from the previous multi-content PR and to this abd hope to have something up running in production within a month utilizing it.

So is there anything you'd like us to test or help out with?
We will start giving community feedback here once we have questions, sugesstions etc.

Excited!

@erezrokah
Copy link
Contributor Author

Thanks @reimertz, just testing it out would be great.
We plan on releasing it very soon.

@erezrokah erezrokah force-pushed the feat/multi-content-authoring-2 branch 2 times, most recently from fdec4cc to 8186d4a Compare September 15, 2020 19:23
@erezrokah erezrokah force-pushed the feat/multi-content-authoring-2 branch from 8186d4a to 671c705 Compare September 20, 2020 16:31
@erezrokah
Copy link
Contributor Author

Here my example with the size. I don't ask to change the PR to support multiple Dimensions (like in my example below) but I ask to have the config already named "right". For the beginning the dimensions only accept locales as definitions and the UI and everything else stays the same.

Thank you @signalwerk for raising this issue.
While dimensions is a valid use case, for the sake of moving this PR forward, I think it might be better to avoid adding another layer of abstraction and a new concept which is less familiar than i18n. Having an API for a feature that is not fully implemented can be confusing.

If/when we add that feature later on we can discuss naming, configuration structure, etc. and backwards compatibility should be handled fairly easily by transforming existing configurations to the new structure.

@erezrokah erezrokah merged commit cb2ad68 into master Sep 20, 2020
@erezrokah erezrokah deleted the feat/multi-content-authoring-2 branch September 20, 2020 17:30
@signalwerk
Copy link

@erezrokah I agree. The transformation of the config is indeed simple. And it could probably even co-exist. Thank you all for the big work went into that feature! I'm super excited!

@tbille
Copy link

tbille commented Sep 21, 2020

Whaaaat this is done! Can't wait to try it 🎉
Thanks for this work @erezrokah

@erezrokah
Copy link
Contributor Author

This is now released in [email protected] and [email protected].
Thank you everyone for your patience and contributions.
Docs are here https://www.netlifycms.org/docs/beta-features/#i18n-support.
Any kind of feedback is appreciated (you can open a new issue for it).

@bernardolima
Copy link

Hello everyone, I'm trying to implement this new feature, but as far as I see the preview only works for the default language. I believe it's related to this:
#4139 (comment)

I have this data variable in the preview, and it only returns the default language's content:
Screenshot from 2020-09-25 18-00-26

Obs: I'm using Netlify CMS with gatsby, and the languages are english and portuguese.

@barthc
Copy link
Contributor

barthc commented Sep 25, 2020

I have this data variable in the preview, and it only returns the default language's content:

@bernardolima I would suggest you open up a new issue.

@chriskirknielsen
Copy link

chriskirknielsen commented Oct 4, 2020

First off, thank you for working on this massive feature, it's amazing to see it in action! 😄

Would there be a way to add an option such as locale_folders to specify the following structure: <locale>/<folder>/<slug>.<extension>? Or is there perhaps a way to do that currently that I've missed? (in which case, please forgive me)

I'm attempting to implement this new configuration into my build, and it's proving to be more cumbersome than I initially anticipated to get it to work with my setup (e.g. en/posts/slug.md). I'm willing to make the switch but wanted to see if the option was on the table before I commit to rewriting my structure.

Thanks!

EDIT: I have seemingly gotten this to work locally by extending I18N_STRUCTURE but it might not work like expected everywhere. I won't lie, I am a total noob when it comes to integration tests and the like, and I don't want to embarrass myself. So here's a gist of the file I edited, i18n.ts with the added support I am aiming for. I'm just trying to help but understand if this is a bit annoying to implement into core.

@erezrokah
Copy link
Contributor Author

Hi @chadwithuhc, do you mind opening a new issue/feature request for this? We can discuss possible solutions there

@chriskirknielsen
Copy link

@erezrokah Created issue #4416 — thanks!

@naton
Copy link

naton commented Nov 13, 2020

Impressive work on this one! Just one quick question, is it possible to store the corresponding <locale> in each post/page as well? Can't read anything here that says so, but wanted to be sure.

@erezrokah
Copy link
Contributor Author

Hi @naton, that is not possible yet. You could extract the locale from the file/folder name.
I recommend opening a feature request for allowing to save it as a field.

@naton
Copy link

naton commented Nov 18, 2020

Thanks! I finally got it to work for me, using scope filters in Jekyll 👍

@alexndr-n
Copy link

alexndr-n commented Apr 8, 2021

Hey, great work on this feature so far! I would just like to ask if there is a way for it to automatically produce translations using something like Libre on the go, and also a way to notify the user which posts are missing certain translations or the translations are up for review. I believe these were mentioned back in #716.

Great work again!

Edit: found it, commented on #716 by @erquhart
#716 (comment)
#716 (comment)
Any workaround at least for seeing which entries have no translations on which languages?

@tonioriol
Copy link

If anyone reaches here looking how to get i18n data in previews here's a test that demonstrates how the translations are structured: 23e2bce#diff-a0137750f308007aeb002ba8fcfb8fa2ab0e4b07eb9678d9baab8e46e3817d61R687

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