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

Enable locstring navigation from LGV import form #1642

Merged
merged 24 commits into from
Feb 4, 2021

Conversation

teresam856
Copy link
Contributor

@teresam856 teresam856 commented Jan 26, 2021

Issue 1543
Follow up from issue 1509

  • Changes the onChange behavior of the refNameAutocomplete to allow import form and header to handle setting region or navigating to locString.
  • Changes the behavior of the onSelect of import form and header

Now able to open an LGV at a specific location from the import form

demo.mov

@github-actions github-actions bot added the needs label triage Needs a label to show in changelog (breaking, enhancement, bug, documentation, or internal) label Jan 26, 2021
@teresam856 teresam856 added enhancement New feature or request and removed needs label triage Needs a label to show in changelog (breaking, enhancement, bug, documentation, or internal) labels Jan 26, 2021
@teresam856 teresam856 marked this pull request as draft January 26, 2021 23:37
@teresam856 teresam856 added the bug Something isn't working label Jan 26, 2021
@teresam856 teresam856 self-assigned this Jan 26, 2021
@teresam856 teresam856 changed the title 1543 locstr from import Enable locstring navigation from LGV import form Jan 26, 2021
@cmdcolin
Copy link
Collaborator

Can you enable it so that hitting the "enter key" performs the submit?

@cmdcolin
Copy link
Collaborator

cmdcolin commented Jan 27, 2021

Also, not really any fault of this PR or anything, just the history of this component, but it is unclear that using locstring is even something we'd be allowed to do because it suggests that we are only selecting "sequence" when really we want it to be a more generic thing that can handle refseq names, locstrings, and even gene features once name indexes are available

@teresam856
Copy link
Contributor Author

Also, not really any fault of this PR or anything, just the history of this component, but it is unclear that using locstring is even something we'd be allowed to do because it suggests that we are only selecting "sequence" when really we want it to be a more generic thing that can handle refseq names, locstrings, and even gene features once name indexes are available

What do you suggest could make this clearer? Changing the helper text or the labels?

@teresam856
Copy link
Contributor Author

Can you enable it so that hitting the "enter key" performs the submit?

Yeah. Could also remove the open button. Then the LGV would open up either by choosing an option from the dropdown or hitting the enter key.

@cmdcolin
Copy link
Collaborator

cmdcolin commented Jan 27, 2021

Regarding making it clearer, in the import form is says

"Select sequence to view" and there is a thing that says "sequence" wrapping the textbox

In the LGV header bar it has no such helper text though

I believe it should either have no helper text, or the helper text it has should say they can enter a sequence or locstring

@codecov
Copy link

codecov bot commented Jan 27, 2021

Codecov Report

Merging #1642 (4a46da3) into master (80e4526) will increase coverage by 0.00%.
The diff coverage is 77.02%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1642   +/-   ##
=======================================
  Coverage   58.60%   58.61%           
=======================================
  Files         444      444           
  Lines       20385    20396   +11     
  Branches     4787     4786    -1     
=======================================
+ Hits        11947    11955    +8     
- Misses       8141     8147    +6     
+ Partials      297      294    -3     
Impacted Files Coverage Δ
...iew/src/LinearGenomeView/components/ImportForm.tsx 71.42% <58.33%> (-0.45%) ⬇️
...me-view/src/LinearGenomeView/components/Header.tsx 83.78% <72.72%> (+7.11%) ⬆️
...s/linear-genome-view/src/LinearGenomeView/index.ts 84.47% <87.87%> (-0.06%) ⬇️
...inearGenomeView/components/RefNameAutocomplete.tsx 89.36% <100.00%> (+1.43%) ⬆️
packages/core/util/index.ts 79.53% <0.00%> (-0.34%) ⬇️
products/jbrowse-desktop/src/factoryReset.ts 0.00% <0.00%> (ø)
...y-jbrowse/src/JBrowse1Connection/jb1ConfigParse.ts 25.88% <0.00%> (ø)
...src/LinearPileupDisplay/components/FilterByTag.tsx 5.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 80e4526...4a46da3. Read the comment docs.

@teresam856 teresam856 marked this pull request as ready for review January 29, 2021 19:38
@rbuels
Copy link
Contributor

rbuels commented Feb 2, 2021

not sure removing the open button would actually be the best user experience, seems like it would be easy for a user to fat-finger and enter the wrong thing, and then end up someplace else and get confused? what do you guys think?

@cmdcolin
Copy link
Collaborator

cmdcolin commented Feb 2, 2021

Ya I don't think I meant to remove the open button, I'd keep the open button but to allow as an option to hit the enter button to do the same action as open basically.

fireEvent.keyDown(autocomplete, { key: 'ArrowDown' })
fireEvent.keyDown(autocomplete, { key: 'ArrowDown' })
fireEvent.keyDown(autocomplete, { key: 'Enter', code: 'Enter' })
expect(model.displayedRegions.length).toEqual(1)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice work on the testing. why are arrowdown needed?

Copy link
Contributor Author

@teresam856 teresam856 Feb 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am testing the alternate search option from the dropdown.
https://github.com/GMOD/jbrowse-components/pull/1642/files#diff-82a164c4727a03a6f61491bcd006b102c68cd5f7ef74c19adfa73671fba37b61R87-R90 .Removed the arrow down and added test for open button.

@teresam856
Copy link
Contributor Author

not sure removing the open button would actually be the best user experience, seems like it would be easy for a user to fat-finger and enter the wrong thing, and then end up someplace else and get confused? what do you guys think?

Yeah, I can work on keeping both the submit on enter and on open click. Will add the button back in. 👍

@rbuels rbuels merged commit 52cce54 into master Feb 4, 2021
@rbuels rbuels deleted the 1543_locstr_from_import branch February 4, 2021 21:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants