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: added frontmatter props #57

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

runyasak
Copy link

@runyasak runyasak commented Dec 7, 2024

Description

I changed the frontmatter from a variable to defineProps with default values.
Now, we can override the frontmatter value by passing props to the component.
The current frontmatter value will be default props value.

Here is an example:

<template>
  <HelloWorld name="My Awesome App"  />
</template>

<script>
import HelloWorld from './README.md'

export default {
  components: {
    HelloWorld,
  },
}
</script>

Linked Issues

Here is the related issue: #45

Additional context

For now, I made it support Vue 3. If you want me to continue this feature, I will make for support Vue 2.


If you have any feedback or suggestions, please let me know.

Copy link

stackblitz bot commented Dec 7, 2024

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@runyasak runyasak mentioned this pull request Dec 7, 2024
3 tasks
@antfu
Copy link
Member

antfu commented Dec 9, 2024

I am not sure if we should expose everything in the frontmatter to be props. This could lead to implicit and unexpected results as we are mixing different concepts as a single object.

I would imagine something like:

<HelloWorld :frontmatter-merge="{ name: 'My Awesome App' }"  />

and

<HelloWorld :frontmatter-replace="{ name: 'My Awesome App' }"  />

might be more explicit and less conflicting

@runyasak
Copy link
Author

runyasak commented Dec 9, 2024

I am not sure if we should expose everything in the frontmatter to be props. This could lead to implicit and unexpected results as we are mixing different concepts as a single object.

I would imagine something like:

<HelloWorld :frontmatter-merge="{ name: 'My Awesome App' }"  />

and

<HelloWorld :frontmatter-replace="{ name: 'My Awesome App' }"  />

might be more explicit and less conflicting

Hi @antfu. This is a great idea. I completely agree with you. It will be more explicit for users and allow for additional props to be added in the future.

Do you think :frontmatter would be suitable for this case?

Could you please suggest the prop name you prefer, :frontmatter, :frontmatter-merge, or :frontmatter-replace?
I’m happy to go with any option you choose.

Let me know, and I’ll update my code as soon as possible.

@antfu
Copy link
Member

antfu commented Dec 9, 2024

I was thinking to have frontmatter-merge and frontmatter-replace at the same time for directly replace, or object merge (to keep other unspecified values), two different behaviors. Or am I over-engineering it a bit?

@runyasak
Copy link
Author

runyasak commented Dec 9, 2024

I was thinking to have frontmatter-merge and frontmatter-replace at the same time for directly replace, or object merge (to keep other unspecified values), two different behaviors. Or am I over-engineering it a bit?

You're not over-engineering it; I just missed the point that both behaviors might be necessary.

Let me check my understanding:
Imagine we have three frontmatter properties: name, title, and meta, each with a value.

  • If we pass :frontmatter-merge="{ name: 'My Awesome App' }", the other frontmatter properties (title and meta) will retain their original values.
  • If we pass :frontmatter-replace="{ name: 'My Awesome App' }", the other frontmatter properties will not render and will default to empty values.
    Is that the behavior you have in mind?

In my opinion, :frontmatter-merge might be sufficient for overriding frontmatter values. I’m not sure users would frequently need to replace the entire frontmatter object. Additionally, they could pass null, undefined, or another value with :frontmatter-merge to hide certain frontmatter properties.

Perhaps we could start with :frontmatter-merge for now. If we receive feedback requesting the ability to replace the frontmatter entirely, we could implement :frontmatter-replace later.

This is just my suggestion. Please let me know your suggestion—I'm happy to follow your decision.

@antfu
Copy link
Member

antfu commented Dec 10, 2024

Yeah, that's exactly what I mean.

The replace could be useful as:

  • It would be tricky to remove keys with merge
  • Keys that value's null or undefined still different from no keys at all. Object.keys() will have different results
  • Overriding keys with undefined requires you to act knowledge of the other keys available. Meaning it's not trivial to do "remove all keys"

@runyasak
Copy link
Author

runyasak commented Dec 10, 2024

I understand. I will update my code to include both :frontmatter-merge and :frontmatter-replace soon.

@runyasak
Copy link
Author

@antfu I've updated my code with :frontmatter-merge and :frontmatter-replace.
Please, review my code again.

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.

2 participants