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

Add compatability with @editorjs/list data format #39

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

cam-perry
Copy link

@cam-perry cam-perry commented Jul 8, 2022

  • Resolves Make it compatible with @editorjs/list #15 ,
  • Upgrading from @editorjs/list to @editorjs/nested-list is a pain because the data format is not compatible.
  • This change detects the items: string[] format from @editorjs/list and converts to the items: Item[] format used here.

README.md Outdated
| Field | Type | Description |
| ----- | ----------------- | ---------------------------------------- |
| style | `string` | type of a list: `ordered` or `unordered` |
| items | `(Item \| string)[]` | the array of list's items |
Copy link
Author

@cam-perry cam-perry Jul 8, 2022

Choose a reason for hiding this comment

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

Let me know whether I should keep this README or if there is another way you prefer to document it.

The current README change feels incorrect because this is under the "Output data" header but the data is never saved with items: string[].

Copy link
Contributor

Choose a reason for hiding this comment

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

I think, we can remove it at all, because it's just a legacy format support, it shouldn't be mentioned in main Readme

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, please increment a patch version

README.md Outdated
| Field | Type | Description |
| ----- | ----------------- | ---------------------------------------- |
| style | `string` | type of a list: `ordered` or `unordered` |
| items | `(Item \| string)[]` | the array of list's items |
Copy link
Contributor

Choose a reason for hiding this comment

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

I think, we can remove it at all, because it's just a legacy format support, it shouldn't be mentioned in main Readme

src/index.js Outdated
Comment on lines 110 to 115
this.data.items = this.data.items.map((item) => {
if (typeof item === "string") {
return { content: item, items: [] };
}
return item;
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Please, move this code to a separate method

README.md Outdated
| Field | Type | Description |
| ----- | ----------------- | ---------------------------------------- |
| style | `string` | type of a list: `ordered` or `unordered` |
| items | `(Item \| string)[]` | the array of list's items |
Copy link
Contributor

Choose a reason for hiding this comment

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

Also, please increment a patch version

- For projects that already used `@editorjs/list`, upgrading
  to `@editorjs/nested-list` is a pain because the data
  format is not compatible.
- This change detects the `items: string[]` format from
  `@editorjs/list` and converts to the `items: Item[]` format
  used here.
@@ -102,6 +102,7 @@ export default class NestedList {
};

this.data = data && Object.keys(data).length ? data : initialData;
this.convertStringDataToListItems()
Copy link
Contributor

Choose a reason for hiding this comment

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

lets do that with data before assigning to this.data

*/
convertStringDataToListItems() {
if (this.data.items && this.data.items.length > 0) {
this.data.items = this.data.items.map((item) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd suggest to skip redundant processing of data in correct format:

if (typeof data.items[1] !== 'string') {
  return;
}

@robonetphy
Copy link
Member

@cam-perry, Thanks for PR. Please update your PR or give us access to your fork so we can complete this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make it compatible with @editorjs/list
3 participants