Skip to content

feature: add label property to DiscreteFilter#82

Merged
okdistribute merged 1 commit intonextfrom
discretefilter-labels
May 1, 2018
Merged

feature: add label property to DiscreteFilter#82
okdistribute merged 1 commit intonextfrom
discretefilter-labels

Conversation

@okdistribute
Copy link
Contributor

fixes #68

@GoGoCarl is this what you were thinking?

@GoGoCarl
Copy link

GoGoCarl commented Apr 25, 2018

@Karissa Not really, but I think I did not completely explain my full intentions in the ticket as I had discussed this with Gregor before.

I've updated the ticket to include a screenshot of what I'm talking about. So, the label I'm looking to customize is for the value of the field:

https://github.com/digidem/react-mapfilter/pull/82/files#diff-e58a7d48fe0412723727ccaef23c2184R154

That said, it WOULD be nice to be able to customize the entire filter heading as well as you have done here, but I think it would be best served if this were possible to do at a translation level. There's a translations map that gets initialized in MapFilter here:

https://github.com/digidem/react-mapfilter/blob/next/src/containers/MapFilter.js#L28-L43

It would be cool to be able to hook into that mapping somehow. Then, I could add my own translations for things like field_key.happening to have it translate in different languages.

@okdistribute
Copy link
Contributor Author

What does your imagined API interface to hook into the translations object look like here?

@gmaclennan
Copy link
Member

How about we allow translations as a prop, and deep-merge with the translations map? We could just have field_key translations and field_value translations, but I don't see harm in allowing all translations to be optionally overwritten. That way the component consumer does not need to wait for translations to be merged into react-mapfilter.

@GoGoCarl
Copy link

@Karissa I think what @gmaclennan described above would work well.

@okdistribute okdistribute force-pushed the discretefilter-labels branch from 06dab48 to 33a71ad Compare April 27, 2018 18:24
@okdistribute
Copy link
Contributor Author

@GoGoCarl ok this should work now!

@okdistribute okdistribute merged commit 721bb64 into next May 1, 2018
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.

Display options for filter labels

3 participants