search

cult-of-coders / grapher-react

Provides easy to use React Components that are suitable for grapher package.
https://atmospherejs.com/cultofcoders/grapher-react
38 stars 19 forks source link

withQuery() does not support dataProp option #14

Closed bhunjadi closed 6 years ago

bhunjadi commented 6 years ago

Hi, I've just updated grapher-react and am in process of cleaning deprecation warnings for createQueryContainer.

But I've noticed that it is not possible to pass dataProp option to withQuery. This is implemented in createQueryContainer here: https://github.com/cult-of-coders/grapher-react/blob/40c0d8a6df1bfafdd97586d4b4a22c1896b7856a/legacy/createQueryContainer.js#L20

My question is if this is a design decision, meaning you don't want to support this parameter anymore? (Haven't seen such notice in changelog, though) Otherwise, I can make a PR.

As I understand it:

  1. checkOptions have to be updated to accept dataProp
  2. withQueryContainer should use that parameter instead of fixed data key; https://github.com/cult-of-coders/grapher-react/blob/40c0d8a6df1bfafdd97586d4b4a22c1896b7856a/lib/withQueryContainer.js#L42
theodorDiaconu commented 6 years ago

@bhunjadi I think we should have it, you're right, there shouldn't be any reason not to. But we have to be careful about it.

bhunjadi commented 6 years ago

Great, I'll try to do the PR next week.

theodorDiaconu commented 6 years ago

@bhunjadi I remember there was a reason for why I avoided it.... I don't remember which though.