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

Dynamic Routing: custom meta title & body_class output for static routes using data key #10082

Closed
kirrg001 opened this issue Oct 29, 2018 · 17 comments
Labels
affects:api Affects the Ghost API affects:routing related to the url service & dynamic routing features bug [triage] something behaving unexpectedly later [triage] Things we intend to work but are not immediate priority server / core Issues relating to the server or core of Ghost

Comments

@kirrg001
Copy link
Contributor

kirrg001 commented Oct 29, 2018

Reported in the forum: https://forum.ghost.org/t/when-using-dynamic-routes-meta-title-and-body-class-are-empty/3765

Custom Meta Title

routes:
   /page1/:
    data: page.SLUG
    template: page

If you use

<head>
    <title>{{meta_title}}</title>
</head>

{{#page}}
{{/page}}

It always outputs the blog meta title and not the custom page meta title.

I see two problems:

  1. the StaticRoutesRouter does not forward the correct context, see. It has to forward the resource type as well (e.g. page) if the data key was provided.

  2. The meta title helper is not aware of the this.page context object -> data.page.

Body Class Helper

{{body_class}}

This helper also needs to respect this.page.

Code suggestion:

page = this.post && this.post.page ? {slug: this.post.slug} : this.page || false;
classes.push('page-' + page.slug);

Workaround

{{#page}}
  {{meta_title}}
{{/page}}

Ghost: master

@kirrg001 kirrg001 added bug [triage] something behaving unexpectedly themes server / core Issues relating to the server or core of Ghost help wanted [triage] Ideal issues for contributors to help with affects:routing related to the url service & dynamic routing features labels Oct 29, 2018
@kirrg001 kirrg001 changed the title Dynamic Routing: custom meta title for static routes does not work Dynamic Routing: custom meta title for static routes & body_class helper Oct 29, 2018
@kirrg001 kirrg001 changed the title Dynamic Routing: custom meta title for static routes & body_class helper Dynamic Routing: custom meta title & body_class output for static routes using data key Oct 29, 2018
@godofredoninja
Copy link

In the dynamic routes also the following fields are empty

— Meta Data
— Twitter Card
— Facebook Card

For example, I have these data added

dos

But he answers me with this data.

uno

@kirrg001
Copy link
Contributor Author

@godofredoninja Thanks. It's for sure the same underlying problem 👍 The helpers should respect the page context object.

@VeloAddict
Copy link

Is there a chance this will be fixed in the next release? :)

@naz
Copy link
Contributor

naz commented Oct 30, 2018

@velobarplus if there's somebody from the community to pick it up and contribute a PR we'd definitely help and merge it asap ;)

@cotko
Copy link
Contributor

cotko commented Nov 12, 2018

@kirrg001 I'm wiling to do this, but what's up with the missing resourceType in the StaticRoutesRouter, where shuold that be used?
Also you say that key should be used for that: page if the data key was provided - what about when there is more than one data key:

routes:
    data:
        page:
           resource: posts
           type: read
           slug: page1
        morepages:
           resource: posts
           type: read
           slug: page-more
        tags:
           resource: tags
           type: browse
           limit: all   

what should the implementation of StaticRoutesRouter.getResourceType() return in this case?

@kirrg001
Copy link
Contributor Author

@kirrg001 I'm wiling to do this, but what's up with the missing resourceType in the StaticRoutesRouter, where shuold that be used?

Nice 👍

We have to push the resource type into the context object.

e.g. resource type is "page" => context = [this.routerName, resourceType]

@kirrg001
Copy link
Contributor Author

Also you say that key should be used for that: page if the data key was provided - what about when there is more than one data key:

If there is more than one data definition, the target url does not represent a single resource. I'd not do anything in this case and don't push the resource type.

@cotko
Copy link
Contributor

cotko commented Nov 12, 2018

@kirrg001 ok so if a post is turned into a page, then data.page is present here in the schema meta helper (now that page is pushed into a context).

This in turns calls getPostSchema method, but the page only contains partial data about the post; missing are

  • tags
  • authors
  • primary_author = null // probably because of missing authors?
  • primary_tag = null // probably because of missing tags?

What to do in this case, should post turned into a page also fetch authors and tags so that full data is present, or should meta data in case of page skip this part ?

@kirrg001
Copy link
Contributor Author

The static controller should probably auto include tags and authors if the resource is post | page.

But i'd suggest to create separate and small pull requests per problem 👍
Thanks for your help!

kirrg001 pushed a commit that referenced this issue Dec 3, 2018
refs #10082

- this is a requirement if a static route represents a single resource

e.g. `data: page.team`

- the page resource will no longer live on it's original static url
- instead, it now lives somewhere else
- that means the whole site needs to act the same than the original static url
  - the resource does not contain any relations
  - we don't forward the correct context (page, post, user?)
- we override the `include` property for now
  - need to wait for more use cases or bug reports for this controller
- more changes will follow asap
@ErisDS ErisDS added the later [triage] Things we intend to work but are not immediate priority label Jan 23, 2019
@ErisDS ErisDS closed this as completed Jan 23, 2019
@ErisDS
Copy link
Member

ErisDS commented Jan 24, 2019

All issues tagged with routing have been tagged with later and closed until we prioritise working on the Routing feature again.

@marekgregor
Copy link

Just for information, workaround is described in detail in https://stackoverflow.com/a/54788950/466677

@chrisspiegl
Copy link

I've noticed this same issue and in my opinion, this basically makes custom routes useless — at least for what I am trying to do (replacing the home page). Technically it works but it's a pain without the {{body_class}} and other parameters being set properly.

I've tried numerous ideas as a workaround but came up empty-handed each time 🙈.

I'm 🤞 for someone to take point on this and get it in on one of the next releases.

@ErisDS ErisDS removed the later [triage] Things we intend to work but are not immediate priority label Mar 11, 2019
@ErisDS
Copy link
Member

ErisDS commented Mar 11, 2019

Re-opening this to handle prioritisation

kirrg001 added a commit that referenced this issue Mar 12, 2019
…routes

refs #10082

```
routes:
  /news/:
    data: post.news
```

The twitter_image was not available, because the context is [news, post] and the data is in `data.post`.
The context helper was incorrect. I think it is still not fully correct, but only focused on this use case.
The meta layer needs a full refactoring.
kirrg001 added a commit to kirrg001/Ghost that referenced this issue Mar 12, 2019
refs TryGhost#10082

- meta_title output wrong meta title

Only solves meta_title outout for this use case:

```
routes:
  /:
    data: page.{slug}
    template: t
```
kirrg001 added a commit to kirrg001/Ghost that referenced this issue Mar 12, 2019
refs TryGhost#10082

- throwed a 500 because this.page was not handled
- v2 differentiates between page and post
kirrg001 added a commit that referenced this issue Mar 12, 2019
refs #10082

- meta_title output wrong meta title

Only solves meta_title outout for this use case:

```
routes:
  /:
    data: page.{slug}
    template: t
```
kirrg001 added a commit that referenced this issue Mar 12, 2019
refs #10082

- throwed a 500 because this.page was not handled
- v2 differentiates between page and post
@kirrg001
Copy link
Contributor Author

Some fixes landed in 2.17.0.

@kirrg001
Copy link
Contributor Author

kirrg001 commented Mar 14, 2019

Further problems I see:

  • the whole meta layer & helper layer needs to be reviewed & updated to understand page and post
  • data: page.{slug} does not provide authors,tags by default, which "breaks" the tags helper (empty output)
  • multiple api versions & routing layer/dynamic routing need to be fully reviewed to work 100% generic. There are still some hard-coded checks inside.

The areas need some love and time.

@kirrg001 kirrg001 added affects:api Affects the Ghost API and removed help wanted [triage] Ideal issues for contributors to help with labels Mar 14, 2019
@kirrg001
Copy link
Contributor Author

kirrg001 commented Mar 20, 2019


Cannot read property 'slug' of undefined

----------------------------------------

TypeError: Cannot read property 'slug' of undefined
    at getPostSchema (/core/server/data/meta/schema.js:72:31)
    at getSchema (/core/server/data/meta/schema.js:179:20)
    at core/server/data/meta/index.js:101:27```
/resources/:
    permalink: /{slug}/
    template: resources
    filter: tag:-news
    order: featured desc, published_at desc
    data:
      post: page.SLUG

The error is not catched/logged properly. Needs investigation why the error is happening.

@kirrg001 kirrg001 self-assigned this Mar 20, 2019
naz added a commit to naz/Ghost that referenced this issue Apr 30, 2019
refs TryGhost#10082

- When specifying an existing page as an allias for collection, e.g: `data: page.it-is-a-page` it was failing to generate metadata
naz added a commit that referenced this issue Apr 30, 2019
refs #10082

- When specifying an existing page as an allias for collection, e.g: `data: page.it-is-a-page` it was failing to generate metadata
@stale
Copy link

stale bot commented Aug 12, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale [triage] Issues that were closed to to lack of traction label Aug 12, 2019
@naz naz added help wanted [triage] Ideal issues for contributors to help with and removed stale [triage] Issues that were closed to to lack of traction labels Aug 12, 2019
@ErisDS ErisDS added later [triage] Things we intend to work but are not immediate priority and removed help wanted [triage] Ideal issues for contributors to help with labels Aug 21, 2019
@ErisDS ErisDS closed this as completed Aug 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects:api Affects the Ghost API affects:routing related to the url service & dynamic routing features bug [triage] something behaving unexpectedly later [triage] Things we intend to work but are not immediate priority server / core Issues relating to the server or core of Ghost
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants