-
Notifications
You must be signed in to change notification settings - Fork 355
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
Adding optional virtual keyboard to searchbox #2952
Adding optional virtual keyboard to searchbox #2952
Conversation
Thanks, @ThoWagen, this looks good to me! Before merging, though, I'm hoping that @crhallberg can take a quick look at the styles and JS code to see if he has any suggestions, and that @sturkel can take a little time testing the functionality in all of the themes to see if she can find any usability or display issues. Once I hear back from them, I'll approve and merge unless there are problems revealed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @sturkel89, for the detailed testing. @ThoWagen, when you return from vacation, can you look into the style issues highlighted in the comments above? I imagine that @crhallberg might be able to assist if you have specific questions about the variations between the themes.
@sturkel89 Thank you for your input! Sometimes the keyboard was not loaded because if the searchbox is rendered inside of layout.phtml the headScripts are already included and the JS for the keyboards is not loaded. I moved the logic to the start of layout.phtml and it should work now. I replaced the names of the keys with arrows because unfortunately they can not be translatable without a lot of more work. I thought the arrow keys would be an international standard. But I might be wrong here. However, if you still prefer "Enter" I can change this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the progress, @ThoWagen -- a couple of minor suggestions on the latest refactoring. Beyond that, please let me know when you're ready for us to take another look at the style issues.
@demiankatz @sturkel89 I fixed the other themes. |
Thanks for the progress on this, @ThoWagen! @sturkel89 is out of the office today, but I'll see if she can find some time to review this again after she returns. |
@ThoWagen I have reviewed the branch in all three themes, and everything seems to display and perform correctly! The keyboard icon looks great within the search box in every theme, and the keyboard itself fits the page well and resizes nicely as you resize the browser window. I saw your note about the arrow symbol rather than the word Enter on the Enter key, and I understand your reasoning. I think it's okay with the arrow; keyboard users will be familiar with the function based on the location of the key and the arrow symbol. I don't have any other critiques - this seems ready to go! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like some conflicts need to be resolved, but you have my approval!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm assigning this PR to the 10.0 milestone. It is very mature and should be safe to merge soon, but I still have a few questions, and since it does make some fairly impactful code changes, I think it may be safest not to merge it so late in the development of release 9.1, in case it introduces unforeseen issues.
See below for a couple of specific questions/suggestions.
Additionally, regarding the general user interface: I was initially confused that selecting a virtual keyboard didn't seem to do anything -- I expected the keyboard to pop up immediately when I chose it, rather than later, when the input area was focused. Would it make sense to focus the input when the virtual keyboard is selected? Additionally, I wonder if it would make sense to have a visual indicator that the virtual keyboard is active (such as a highlighted color on the icon) when the setting is something other than "none".
What do you think?
The keyboard is now shown on selection and also the selection button is highlighted if a keyboard is selected, as I also think this makes using it less confusing for users. |
I am not sure why the security check fails. The tests run with no errors locally and I don't have access to view the details. |
@ThoWagen, the security check is failing because it can't run while there are conflicts on the branch. I think you just need to merge dev, run |
@ThoWagen, apologies for introducing more conflicts, but it looks like we need to resolve this with the changes in #3080, where I borrowed some of your ideas from here to clean up our reset button focus problems. I hope that after the merge, the code here will be slightly simplified. Please let me know if you need any help from my end or if you have any questions! |
@demiankatz I resolved the conflicts. But I basically just have overwritten the changes of #3080 again. You mentioned in your comment that this code might be simplified. But I'm not sure what you mean. Did I miss something? |
Thanks, @ThoWagen. I thought perhaps more of the existing code from #3080 might be applicable here, in which case it would reduce the number of diffs found in this PR... but if that doesn't make sense, then I have no objection to what you have done... it will just require some more thorough testing to be sure all the functionality remains consistent. At the moment I'm focused on finishing up remaining 9.1-targeted work, but I hope to get to this fairly early in the 10.0 development process. Apologies again that things are not moving more quickly -- there's a great deal going on at the moment. :-) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ThoWagen, now that 9.1 development has settled down, I'm back to reviewing 10.0 PRs. I see that this one needs some conflict resolution. Do you have a moment to take care of that? If so, I'll give it another look once we have a green checkmark again. :-)
I updated the branch and now it should work again. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking very good to me. I just had one thought on documentation, plus we need to figure out how to resolve the translation piece (see details below).
…agen/vufind into pull-request/keyboards-in-searchbox
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this is ready now -- thanks for all the work on it! I'm merging now, and will work on the translation piece as part of #3200 when time permits. I'm also open to a follow-up to add advanced search support, as discussed earlier, if and when you'd like to work on that.
There are libraries or services that have records in many different languages or even specialize in a specific language. For those, it it nice to be able to search in the original language. However, the languages might use completely different character sets. Therefore, this PR includes the option to configure virtual keyboards for the searchbox.
The library https:/hodgef/simple-keyboard is used and all of the layouts https:/simple-keyboard/simple-keyboard-layouts are supported. The layouts can be activated by uncommenting them in the searchbox.ini. The keyboards can then be selected via a dropdown menu in the searchbox. If no layout is activated in the configurations the dropdown menu is not shown. Hence, this is completely optional.