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(metas): add other tags field #604

Closed

Conversation

niklasmtj
Copy link
Contributor

@niklasmtj niklasmtj commented May 3, 2024

Description

This PR adds the possibilty to add custom meta tags via the metas plugin. It is possible via the other attribute in the metas field.

An code example:

metas:
  title: "My great blog post"
  other:
    "twitter:label1": Reading time
    "twitter:data1": 1 minute

Which results in:

<meta name="twitter:label1" content="Reading time">
<meta name="twitter:data1" content="1 minute">

The background of this PR was adding twitter:label1 and twitter:data1 meta tags to use the preview features that are now available to build previews with. It uses the twitter prefix but is also available on e.g. Slack.

Examples:

X

levelsio on X

See Nomad Score and Nomad Cost here.

Slack

Add support for twitter label meta

From an implementation point of view I took the way that NextJS does it to make them able to add them via the other attribute in the metadata fields. See this discussion

I'm open for feedback to get this feature landed ☺️

Related Issues

none - yet

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.

@niklasmtj
Copy link
Contributor Author

After having another look on the getDataValue function I think we have to extend it to be able to work with arrays. Right now I'm not supporting the possibilty to use something like =others. But I was not 100% how the best way to implement would look like.

@oscarotero
Copy link
Member

oscarotero commented May 3, 2024

Thank! Your implementation looks great! The only problem is the metas object has one more level of depth that will cause merging issues.

For example, if in the root _data.yml file you have some defaults:

metas:
  other:
    "twitter:label1": Reading time

And you want to customize the value in a nested _data file:

metas:
  other:
    "twitter:data1": 1 minute

The complete other object will be overrided, losing the twitter:label1 key, because Lume only merges the first level of the object.

I can think of different solutions:

  1. Allow extra keys in the root of the metas object. Any key different to the standard keys will be added as is.
metas:
  title: "My great blog post"
  "twitter:label1": Reading time
  "twitter:data1": 1 minute
  1. Or create a new mode to merge objects recursively. For example recursiveObject and assign this mode by default to the metas key.

I guess the solution 1 is the most simple to implement, but I would like to hear your opinion.

@niklasmtj
Copy link
Contributor Author

Oh thanks for the feedback. This totally makes sense! I forgot about the merging "only" on the first level.

I think extending the MetaData with something like

interface MetaData {
...
   [key: string]: string
}

To get it to accept dynamic keys in the type. Based on the stack overflow answer. And then maybe destructure the incoming object to get the non fixed defined keys.

I have to try it in the next few days that might be a good idea. Sounds like a first plan to start with. Thanks!

@oscarotero
Copy link
Member

@niklasmtj Hey there! Have you been able to make any progress in this? No problem if you need more time, just want to know it because I want to release a new Lume version soon and I'd like to include this feature. Or let me know if you are not going to work on this (because reasons) so I can continue your work.
Thanks!

@niklasmtj
Copy link
Contributor Author

@niklasmtj Hey there! Have you been able to make any progress in this? No problem if you need more time, just want to know it because I want to release a new Lume version soon and I'd like to include this feature. Or let me know if you are not going to work on this (because reasons) so I can continue your work. Thanks!

Hey sorry, had a busy week. We can also collaborate on this! So if you have an idea on how to implement it easily, go forward :)

@oscarotero oscarotero changed the base branch from main to feat/other-tags-in-metas-plugin May 15, 2024 16:52
@oscarotero oscarotero changed the base branch from feat/other-tags-in-metas-plugin to main May 15, 2024 16:53
@oscarotero
Copy link
Member

@niklasmtj I cannot commit to your PR (obviously) so created this new branch and PR #608 with your commits and additional changes to make the metas object flat.
So I'm closing this. Thanks so much!

@oscarotero oscarotero closed this May 15, 2024
@niklasmtj niklasmtj deleted the feat/other-tags-in-metas-plugin branch May 15, 2024 17:03
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.

None yet

2 participants