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

Trigger autocomplete with paste as well as typing #354

Closed
AmandaBirmingham opened this issue Oct 2, 2018 · 4 comments · Fixed by #569
Closed

Trigger autocomplete with paste as well as typing #354

AmandaBirmingham opened this issue Oct 2, 2018 · 4 comments · Fixed by #569

Comments

@AmandaBirmingham
Copy link
Collaborator

Currently, pasting a sample id into the sample plating page does NOT trigger the autocomplete.

To reproduce:

  1. Go to the sample plating page and make a new plate
  2. using the test database, choose the "Identification of the Microbes for Cannabis Soils" study
  3. In one of the wells, type in (do not cut and paste) 640202
  4. See that autocomplete correctly brings up the sample with this id:

screen shot 2018-10-02 at 10 29 48 am

5) In another well, *paste in* (do not type) 640202 6) See that autocomplete is not triggered, there is no option to select the correct sample with this id, and the sample is therefore shown as unknown:

screen shot 2018-10-02 at 10 30 09 am

@AmandaBirmingham
Copy link
Collaborator Author

Note that this behavior (of not triggering autocomplete on a paste) is NOT the expected behavior of SlickGrid, the library that the plating grid is based upon. To show that SlickGrid does trigger autocomplete on a paste:

  1. Go to http://6pac.github.io/SlickGrid/examples/example-autocomplete-editor.html
  2. Pick any of the "Country of Origin" cells, double-click to enter the edit mode, and erase the contents
  3. Paste in (do not type) Latvia
  4. See that the autocomplete option for "Latvia" pops up:

screen shot 2018-10-02 at 10 38 02 am

@charles-cowart
Copy link
Collaborator

Taking over from @wasade.

fedarko added a commit to fedarko/LabControl that referenced this issue Aug 15, 2019
This follows the solution described by @wasade in biocore#464.

Two accompanying warts (both indicated in comments, search
plateViewer.js for "TODO:") will have to be fixed before I want to
merge this back into the main repo.
@charles-cowart
Copy link
Collaborator

Adding @fedarko , as he's working on getting multicell-cut and paste to behave harmoniously with single cell cut and paste.

Updating this issue w/text from Marcus's email update:

I spent some time working with SlickGrid to see if there were any ways to get around the issue described here (and in Daniel's corresponding PR here), in order to make the CellExternalCopyManager SlickGrid plugin play nicely with google sheets/excel pasting. I wasn't able to reach the ideal solution of having all plugins on at the same time; my guess is that the problem is inherent to the way the CECM plugin works, since it (per its docs) adds another layer of indirection to SlickGrid input.

Anyway! Since that path didn't work out, I implemented the solution that you and Daniel agreed on in this PR. I can confirm that it seems to work properly (the autocomplete-from-a-single-pasted-ID use case Amanda described in her issue is handled properly when the checkbox is disabled, and multi-select pasting is handled properly when the checkbox is enabled). Attaching a screenshot of what the first use case looks like on my system.

Code Changes
The code changes for this fix are relatively small (all of the touched JS code in my fork is here), but there are two accompanying warts that I believe will turn into bugs if left unaddressed. In particular:
I'm pretty sure the way the plugin is registered will mess things up if the browser back button is used while the checkbox is unchecked.
This is essentially the same problem as in #562, wherein the default "state" of the page is not concordant with what the browser imposes when it tries to recreate things on the back button being pressed.
Will probably take a few hours? total to devise+implement a fix for this. (I can probably do it quicker if you need, but don't want to overpromise.)

The PlateViewer initialize() method is getting called twice (verified this—if you put in a console.log('hi') statement at the start of initialize(), it'll get output to the dev console twice, once on the plate configuration being selected and once after the Create button is pressed).
The consequence of this is that this binding is set up twice (so we're calling registerPlugin() twice, and unregisterPlugin() twice). The argument to these methods is the same, though.
I don't think this will cause any bugs, but I'm still nervous about it. There are probably some ways around this, but I'm not sure what fits best with LabControl's design—I'll come pester you about this tomorrow for a minute. I have a list of candidate ideas for solving this in the linked code from my fork above.
Takeaways
Now that we know that multi-select can work mostly in harmony with the autocomplete-from-a-pasted-ID use case, the initial hurdle to resolving #553 is close to being addressed. Once the two "warts" above are addressed, I can submit a PR for these changes. Following that, I'll move on to designing a somewhat-elegant solution for "matching" copied-in IDs with study IDs as we discussed this afternoon (and as described in #532).

Screen Shot 2019-08-14 at 10 55 41 PM

@charles-cowart
Copy link
Collaborator

@fedarko Thanks again for investigating! Yeah, I think it's totally worth it to remove the warts while you're there.

fedarko added a commit to fedarko/LabControl that referenced this issue Aug 17, 2019
Closes biocore#567, and addresses one of the two "warts" needed before my
fork can be merged back in (that will take care of biocore#354)
fedarko added a commit to fedarko/LabControl that referenced this issue Aug 20, 2019
This checks to see if the multiSelectCheckbox is or isn't checked
(it defaults to being checked, but it can be unchecked if the user
uses the back/fwd buttons of their browser to return to this page
after unchecking it).

This is the second of the "warts" remaining in my solution of biocore#354,
so my fork should now be ready for a PR.

Also, I'm gonna tag biocore#562 in this -- this sort of solution might
work there (although just using $(document).ready to force the <select>
to start on `Choose plate type...` would probably be a better option).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment