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.

Sign up for GitHub

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

Jump to bottom

feat: add JSON_LD plugin #687

Merged
merged 1 commit into from
Nov 9, 2024
Merged

feat: add JSON_LD plugin #687

merged 1 commit into from
Nov 9, 2024

Conversation

shuaixr
Copy link

@shuaixr shuaixr commented Nov 7, 2024 •
edited
Loading

Description

add JSON_LD plugin

Related Issues

Fixes #453

Check List

  • Have you read the
    CODE OF CONDUCT
  • Have you read the document
    CONTRIBUTING
    • One pull request per feature. If you want to do more than one thing,
      send multiple pull request.
    • Write tests.
    • Run deno fmt to fix the code format before commit.
    • Document any change in the CHANGELOG.md.

@shuaixr
Copy link
Author

shuaixr commented Nov 7, 2024

@oscarotero How do you think about this? I checked the types in schema-dts and found that all keys except those starting with @ are optional, so I removed objects that only have @type keys during the recursion.

oscarotero
oscarotero requested changes Nov 7, 2024
View reviewed changes
Copy link
Member

@oscarotero oscarotero left a comment

Choose a reason for hiding this comment

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

Wow, that was fast!
Thank you. I just left some comments but it looks great overall!!

plugins/json_ld.ts Outdated Show resolved Hide resolved
plugins/json_ld.ts Outdated Show resolved Hide resolved
plugins/json_ld.ts Outdated Show resolved Hide resolved
plugins/json_ld.ts Show resolved Hide resolved
plugins/json_ld.ts Outdated Show resolved Hide resolved
plugins/json_ld.ts Outdated Show resolved Hide resolved
plugins/json_ld.ts Outdated Show resolved Hide resolved
@shuaixr
Copy link
Author

shuaixr commented Nov 7, 2024

lume/plugins/json_ld.ts

Lines 92 to 102 in e15d18c

declare global {
namespace Lume {
export interface Data {
/**
* JSON_LD elements
* @see https://lume.land/plugins/json_ld/
*/
json_ld?: JsonldData;
}
}
}

@oscarotero Which naming convention should I use here? I’ve seen two different styles in decap_cms.ts and source_map.ts

@oscarotero
Copy link
Member

oscarotero commented Nov 7, 2024 •
edited
Loading

@shuaixr I use snake_case for filenames and camelCase for variables.

Edit: decap_cms should be camelCase, you're right. Unfortunately, it can't be changed now.

@shuaixr shuaixr force-pushed the feat/add_jsonld_plugin branch 2 times, most recently from 39e4b2f to 00f204f Compare November 8, 2024 03:50
@shuaixr shuaixr changed the title [WIP] feat: add JSON_LD plugin feat: add JSON_LD plugin Nov 8, 2024
@shuaixr shuaixr marked this pull request as ready for review November 8, 2024 03:52
@shuaixr shuaixr force-pushed the feat/add_jsonld_plugin branch 3 times, most recently from 1348b15 to 4780bd3 Compare November 8, 2024 04:07
oscarotero
oscarotero requested changes Nov 8, 2024
View reviewed changes
Copy link
Member

@oscarotero oscarotero left a comment

Choose a reason for hiding this comment

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

Good work. thanks!
I just left a couple of comments with additional tweaks.

plugins/json_ld.ts Outdated Show resolved Hide resolved
plugins/json_ld.ts Outdated Show resolved Hide resolved
plugins/json_ld.ts Show resolved Hide resolved
tests/__snapshots__/json_ld.test.ts.snap Outdated Show resolved Hide resolved
@oscarotero oscarotero changed the base branch from main to 2.5.0 November 9, 2024 11:39
@oscarotero oscarotero added this to the 2.5.0 milestone Nov 9, 2024
@oscarotero oscarotero merged commit 2d694c9 into lumeland:2.5.0 Nov 9, 2024
6 checks passed
@oscarotero
Copy link
Member

oscarotero commented Nov 9, 2024 •
edited
Loading

It looks amazing. Thanks so much!!
I changed the base branch to 2.5.0, because this is a new plugin, so it should be released on a minor version. I just released 2.4.0 and 2.4.1 this week and It's likely new patch versions to be released before 2.5.0.

Meanwhile, if you want to use, you can configure the lume/ import to https://cdn.jsdelivr.net/gh/lumeland/lume@9aef52ab8518b2545066bda71861e80185bfe1ac/

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

@oscarotero oscarotero oscarotero approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
2.5.0
Development

Successfully merging this pull request may close these issues.

Add JSON-LD plugin
2 participants
@shuaixr @oscarotero