styxlab / gatsby-theme-try-ghost

A Gatsby theme to build flaring fast blogs from headless Ghost CMS
MIT License
136 stars 56 forks source link

Feat/issue 272 #273

Closed chancharles closed 3 years ago

chancharles commented 3 years ago

TBH, after reading more code, I think this PR doesn't really fit the intention of this module.

I think it should be the consumer's responsibility to adjust its GraphQL to limit what should is used for the theme. However, this is not easily achievable with the Gatsby GraphQL setup (at least, not with my level of understanding). So, for my immediate need, I am just hacking at the source. Feel free to decline this PR if you find it too hacky.

styxlab commented 3 years ago

I am generally in favor of this feature. As the source plugin can be viewed as a standalone package making it more flexible is a great idea. I can approve this change after the following changes/additions have been made:

  1. Document the new option in the README of this package
  2. Make the code backward compatible (if one does not have the new option in the config, code should not crash)
  3. Please remove the ability to filter the "settings". I find this too dangerous as some fields are required. Alternatively, make sure required fields cannot be filtered away.

Thanks a lot for this contribution :-)

chancharles commented 3 years ago

Great. Let's discuss the API of this option. Are you okay with this?

fetchOptions: {
  post: {
    filter: (post) => boolean
  },
  page: {
    filter: (page) => boolean
  },
  setting: {
    map: (setting) => setting
  }
}
styxlab commented 3 years ago

I would prefer it simpler:

filter: {
  posts: (post) => boolean,
  pages: (page) => boolean
} 

And not support filtering the settings, unless you bring very good reasons for it.

chancharles commented 3 years ago

@styxlab Please review the PR again.

chancharles commented 3 years ago

I like the idea but I will get a complaint for filter = { posts: (post) => true, pages: (page) => true }):

'post' is declared but its value is never read.ts(6133)

styxlab commented 3 years ago

I like the idea but I will get a complaint for filter = { posts: (post) => true, pages: (page) => true }):

'post' is declared but its value is never read.ts(6133)

filter = { posts: () => true, pages: () => true }) should do.

chancharles commented 3 years ago

Alright. Please review again.

styxlab commented 3 years ago

Maybe posts.filter(filter.posts) is fine, but please make it consistent in all places. With renaming it should look like: posts.filter(customFilter.posts)

chancharles commented 3 years ago

No problem. Please check again.

styxlab commented 3 years ago

Looks great! Thanks so much for improving this package 💙

chancharles commented 3 years ago

Thanks for your time!