Skip to content

Conversation

@Mister-Hope
Copy link
Member

No description provided.

Comment on lines +30 to +32
extendsPageData.forEach((item) =>
Object.assign(data, { item, meta: { ...data.meta, ...item.meta } })
)
Copy link
Member Author

@Mister-Hope Mister-Hope Nov 7, 2021

Choose a reason for hiding this comment

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

Here I deep merge meta key to avoid the frontter edits being coverd.

But, I think there are issues with this design. I am explaining why there are problems in #519 .

Though this simple change avoid #519, but users are not able to delete any meta keys.

@Mister-Hope
Copy link
Member Author

Mister-Hope commented Nov 16, 2021

I think it's ready for the feature, improvements can make later together with "frontmatter" when having later discussions

@meteorlxy
Copy link
Member

Sorry for delay. (Really busy and kind of tired these days 😅 ) I'll pick up this PR soon.

const route = useResolveRouteWithRedirect('/')

console.log(page.value.meta.foo) // 'bar'
console.log(route.meta.path) // '/'
Copy link
Member

Choose a reason for hiding this comment

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

As meta is already available in the route.meta, is it necessary to put meta in to page data?

I think we can make meta a property of the Page object, and don't need to put it into page data.

What's your opinion? @Mister-Hope

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds ok to me.

Copy link
Member

@meteorlxy meteorlxy Dec 8, 2021

Choose a reason for hiding this comment

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

If we remove it from page data, we could not use extendsPageData to modify it. I have two proposals:

  1. Let users to modify it manually in onInitialized hook:
onInitialized: (app) => app.pages.forEach((page) => {
  page.data.foo = 'foo'
  page.routeMeta.title = page.title
  // ...
})

In fact, the extendsPageData hook could also be removed as we have added page.data property, and users can modify it in this way, too. 😅

  1. Rename extendsPageData to extendsPage:
extendsPage: (page) => {
  page.data.foo = 'foo'
  page.routeMeta.title = page.title
  // ...
}

The only difference is that plugins / themes don't need to traverse the app.pages array by themselves.


@Mister-Hope

@meteorlxy
Copy link
Member

I've replaced extendsPageData with extendsPage in 827a873.

Then this PR could be implemented in a different way.

@meteorlxy meteorlxy closed this Dec 15, 2021
@meteorlxy
Copy link
Member

meteorlxy commented Dec 16, 2021

Implemented in 93cdb53

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