search

monarch-initiative / phenogrid

The phenogrid widget
13 stars 14 forks source link

Fixes bug when comparing to one target, removes fragment/curie switching #260

Closed kshefchek closed 7 years ago

kshefchek commented 7 years ago

This PR fixes the bug related to displaying column counts when there is a single target. It also removes the target type tooltip when there is one target (otherwise a message appears that says "show results for all species." It also removes all of the code that switches an ID between curie and fragment syntaxes.

Note to determine if we have searched for >1 target I am using: if (self.state.owlSimFunction === 'search' && self.state.targetSpecies === 'all') {}

which seems suboptimal, @yuanzhou @cborromeo is there a better way?

Also when setting a limit of 100, the result count currently shows (>100), is this intended? If not it would be easy to fix.

Fixes https://github.com/monarch-initiative/phenogrid/issues/259 Fixes https://github.com/monarch-initiative/monarch-app/issues/1391

yuanzhou commented 7 years ago

As far as I can remember, the "search" mode is only applied to Analyze Phenotypes. At this moment, users can only choose one single species or All from the dropdown menu, so 

if (self.state.owlSimFunction === 'search' && self.state.targetSpecies === 'all') {}

does the check accordingly. But once we make changes to the Analyze Phenotypes UI, say using checkboxes instead of dropdown menu, we'll need to change this if statement again. But since self.state.targetSpecies is only a parameter in the "search" mode, we can compose a list of selected taxa and assign the value to this parameter. Then change some code accordingly. It should be fine for now.

capture

In terms of the "(>100)" question, I think @cborromeo made that change, if it's easy to fix, please do so. And I can merge this PR after this fix.

kshefchek commented 7 years ago

Okay great! I think @jmcmurry helped work on the result count as well, is the intention that if the limit is set to 100 and we get 100 results, there are a likely more and therefore we want ">100"?

kshefchek commented 7 years ago

According to @jmcmurry when should leave the >100 when the limit is set to 100 and we get back 100 results. So this should be good to merge if it looks good to you.

yuanzhou commented 7 years ago

Sounds great. I'll merge in a minute then bump the npm version. Should I also update the package.json in monarch-app so the changes can be reflected on beta?

kshefchek commented 7 years ago

Let's bump it on a branch and make a PR to run the tests just to be on the safe side.

kshefchek commented 7 years ago

I also noticed when working on this that we have a .gitignore.swp file that made its way to the git repo.