Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

load case insensitive data to the tree #17

Merged
merged 18 commits into from
Mar 2, 2017

Conversation

atarkowska
Copy link
Member

@atarkowska atarkowska commented Feb 22, 2017

This PR relaxe queries to load case insensitive data https://trello.com/c/M3q00BhI/55-mapr-case-insensitive-searching
google chromescreensnapz003

Test:

  • Case sensitive is only enabled in gene and compound tab (configurable)
  • Check:
    • mapr/gene/?value=cdc14
    • mapr/gene/?value=cdc14&case_sensitive=true
    • mapr/gene/?value=cdc14&case_sensitive=true&query=true
      case_sensitive & query are not mutually exclusive
  • type cdc14 in autocomplete and test Match case
  • check if tests passes https://idr-ci.openmicroscopy.org:8443/job/MAPR-test/
  • check help tooltip in ? button

@atarkowska atarkowska closed this Feb 22, 2017
@atarkowska atarkowska reopened this Feb 22, 2017
@manics
Copy link
Member

manics commented Feb 23, 2017

Does this need to be an option or could it be simplified so it's always case-insensitive?

@atarkowska
Copy link
Member Author

atarkowska commented Feb 23, 2017

by default it is case insensitive, the option is to filter with case sensitive, see the example testing links

@manics
Copy link
Member

manics commented Feb 23, 2017

I know, I was wondering whether it could always be case-insensitive to reduce the code complexity.

@atarkowska
Copy link
Member Author

atarkowska commented Feb 23, 2017

I don't know enough use cases to judge. Up to @eleanorwilliams, we briefly discussed that both options may be needed.
This is a breaking change anyway!

@eleanorwilliams
Copy link

@manics The casing of the gene symbols is to a certain extent different in different species e.g cdc14 in Drosophila, CDC14 in yeast so if you want to search for the gene in an narrower range of species you can do it using casing.

@eleanorwilliams
Copy link

I find it a little disconcerting that the autocomplete list shows the different casing options, but when i select one it gives me back all of them. Maybe we need a check box that says 'return case-insensitive results' to make it more obvious that its going to do this.

The good thing is that in the results it separates the results e.g. cdc14 and CDC14 are listed separately, so that even though the search is case insensitive you can still refine by the the more specific results.

@eleanorwilliams
Copy link

of course we also have the issue that the Gene Symbols are not always entered consistently - http://idr-ci.openmicroscopy.org:1880/web/mapr/gene/?value=YFR028C

@atarkowska
Copy link
Member Author

@eleanorwilliams thanks for your suggestion, I will update the UI

@atarkowska
Copy link
Member Author

@eleanorwilliams I am done, ready for review. Please read description for reference of new changes.

@eleanorwilliams
Copy link

I like the check box. I personally would not have it greyed out, I'd just have it obvious from the start. And I'd change it to 'Match case'. Otherwise all looks fine.

@atarkowska
Copy link
Member Author

@eleanorwilliams thanks for feedback. updated

@atarkowska
Copy link
Member Author

atarkowska commented Feb 26, 2017

@atarkowska
Copy link
Member Author

@eleanorwilliams
Copy link

Works fine. I noticed its only for genes and compounds (which are the two with issues) but any reason it couldn't be consistent across all mapr types?

Also the other thing that is slightly annoying is that if you check the box after you start typing the search goes away. E.g. if you type cdc14 in to the gene box, see that there are lots of casing options and then click the checkbox to say you want it to be case sensitive, then the cdc14 that you typed is gone. I don't know if there is an easy way round this.

@atarkowska
Copy link
Member Author

as mentioned in the description availability of checkbox is configurable, it was done on purpose to allow testing and exploring both possibilities.

autocomplete is only triggered on typing, so match cases has to wipe a state of autocomplete otherwise it won't work. We can review that in a future.

@pwalczysko
Copy link
Member

The tests https://idr-ci.openmicroscopy.org:8443/job/MAPR-test/ are not accessible to me. Site asks for credentials, and the most obvious username/pwd combinations fail for me.

@pwalczysko
Copy link
Member

All works as expected except

  • the test I could not access load case insensitive data to the tree #17 (comment)
  • the help-tooltip
    • I think needs some reformulation. It says it autocompletes as you type - I am not sure this is accurate, it actually drops down a menu, it does not autocomplete the typed word, you have to select from the dropdown.

    • Gene Symbols (Gene Symbols, Gene Identifiers) --> Gene Symbols (or Gene Identifiers)

@atarkowska
Copy link
Member Author

thanks for testing, please provide final wording for both tooltips :)

@pwalczysko
Copy link
Member

Please select "Match case" or not as desired, then start typing to search for Gene symbol or Gene identifier. Select the desired item from the dropdown menu which appears after typing has started.

The second tooltip on "Match case" is either fine or I would remove it completely, it does not really need a reformulation for me, or even be there (maybe @eleanorwilliams will have a better opinion)

@eleanorwilliams
Copy link

Text from @pwalczysko is good. I think for the mapr categories where you can't select the match case box the tool tip shouldn't mention it. In those cases just have 'Start typing to search for siRNA identifier or siRNA pool identifier. Select the desired item from the dropdown menu which appears after typing has started.' etc.

@eleanorwilliams
Copy link

Tools tips are good now. @pwalczysko what do you think?

@pwalczysko
Copy link
Member

Happy with the tooltips too, thank you.

@atarkowska
Copy link
Member Author

@joshmoore is yours now :)

@joshmoore
Copy link
Member

Excellent! Demo tomorrow with @jrswedlow at the meeting and then let's get it in. Thanks, all.

@joshmoore
Copy link
Member

From @jrswedlow:

Really like the change— so yes, please move into production. I can’t detect any issues with performance, but that might just be me.

@joshmoore joshmoore merged commit 5212b58 into ome:master Mar 2, 2017
@atarkowska
Copy link
Member Author

thx everyone

@atarkowska atarkowska deleted the case_insensitive branch March 2, 2017 16:42
@joshmoore
Copy link
Member

Released as 0.1.11

@sbesson sbesson added this to the 0.1.11 milestone Dec 5, 2017
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.

6 participants