search

statgen / locuszoom

A Javascript/d3 embeddable plugin for interactively visualizing statistical genetic data from customizable sources.
https://statgen.github.io/locuszoom/
MIT License
156 stars 29 forks source link

The parseFields substitution doesn't correctly work with conditions #160

Closed ghpxi closed 5 years ago

ghpxi commented 5 years ago

From this method: https://github.com/statgen/locuszoom/blob/develop/dist/locuszoom.app.js#L398

Documentation says that condition format should be like this: {{#if {{field_name}} }} Conditional text {{/if}} But regexp match only this format: {{#if field_name}} Conditional text {{/if}} It works correcly, but you should either update documentation or fix regex

You can see the tests here: https://regex101.com/r/bU8J04/2

abought commented 5 years ago

Thanks for catching this!

The short answer is that the second version is the correct and intended grammar; I've updated the source code docs accordingly in f096851.

That said, one place where curly braces are still valid is when using namespaces in raw layouts, eg: {{#if {{namespace[catalog]}}rsid}}. The reason is that {{namespace[datasource_name]}} will get resolved when the layout is first retrieved, so that what parseFields eventually sees is {{# if catalog:rsid }}.

abought commented 5 years ago

Also, a small request: we're starting to collect a list of places where LocusZoom.js is used. (including links to any that are public)

If you're willing to share, we'd be interested to see how your usage turns out! The idea is to get a sense of how people are using the tool, and what could be improved.

ghpxi commented 5 years ago

Thank you for the quick answer. By the way, the regex doesn't accept space after a variable. You can check it here. This small thing is confusing. So, the documentation is still invalid.

The place where we use LocusZoom.js isn't public, so I cannot share it, at least for now.

abought commented 5 years ago

Thanks for the feedback!

Indeed, that was fixed in a followup commit. (let me know if it's still in error). https://github.com/statgen/locuszoom/blob/develop/assets/js/app/LocusZoom.js#L360

I've also added the documentation in a more officially visible place on our wiki; it seems this was a hidden feature: https://github.com/statgen/locuszoom/wiki/Layouts#conditional-expressions

If your tool does become public of course let us know; we're happy to get feedback from any channel.

We're thinking of sending out a user survey at some point to find out how people use LZ.js (and what sorts of improvements would help them for common use cases)... if you're on our google group mailing list, we would definitely announce it there.

ghpxi commented 5 years ago

One more place where you can see conditional expressions is Data Layer/tooltip.html section

abought commented 5 years ago

Aha! Thanks! I was looking for that. The docs are probably due for some reorganization.

The linked page has been fixed.