Fix keyboard thumbnail activation (fixes #1068)

[danihagosbl]

This PR addresses #1068.

Description of what you did:

Inside the contents left sidebar users can activate images from the thumbnails. They can tab through and then activate the image to make it display in the image viewer.

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
universalviewer	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Oct 23, 2024 0:04am

[demiankatz]

@danihagosbl, I think this may be a little too broad: it now appears that ANY keypress will cause the thumbnail to activate. This has a couple of side effects:

1.) As soon as you tab to a thumbnail, it immediately activates; I'm not sure if this is desired/intended.

2.) If you press a series of random keys while a thumbnail is highlighted, the image panel reloads for each keypress.

Would it be better to only load the thumbnail if the key being pressed is Enter? Does @scoutb-cogapp have any thoughts/recommendations on desired behavior?

[danihagosbl]

@demiankatz I have fixed this issue, now it loading the thumbnail if the enter key is pressed.

[demiankatz]

Thanks, @danihagosbl, the functionality here looks great!

Just one request before we merge this: it looks like a lot of unnecessary comments, and some extra whitespace, have been added through the pull request. If you look at the "files" tab of this PR, you'll see what I mean. Can you please delete these unnecessary lines to tidy up the commit? (If you need help, I'm also happy to do it!)

Thanks for fixing this issue!

[danihagosbl]

Thanks, @danihagosbl, the functionality here looks great!

Just one request before we merge this: it looks like a lot of unnecessary comments, and some extra whitespace, have been added through the pull request. If you look at the "files" tab of this PR, you'll see what I mean. Can you please delete these unnecessary lines to tidy up the commit? (If you need help, I'm also happy to do it!)

Thanks for fixing this issue!

Thanks @demiankatz for the feedback, I have removed blank spaces and commented code for those I added.

[demiankatz]

Thanks, @danihagosbl! This looks good to me now, but before we merge it, we should reconcile the changes here with the related work in #1164; I will wait to hear what @LlGC-jop has to say about this PR before merging it.

[LlGC-jop]

This looks fine to me.

The main difference I can see between this and #1164 is that the logic for handling which key it is in the react code here, whereas in mine the component just passes the event out to be handled in ContentLeftPanel.ts

That and in mine the spacebar is also a trigger. I suppose the question is do we treat thumbnails as buttons or links? I would argue that as they respond to interaction to change the view without taking the user anywhere else that they're essentially a button, just one with an image rather than text.

We can bring it up on the call, but I'm happy to revert my changes (apart from the accessible click change) in favour of these once we've decided.

[demiankatz]

@LlGC-jop, how would you feel about just merging this now and then making any behavioral adjustments in #1164 when you resolve conflicts, if it's decided on the call that the behavior should change? That way, @danihagosbl gets his work contributed to the project, your PR gets a little bit smaller/simpler, and we hopefully still end up in the same place at the end.

[LlGC-jop]

@demiankatz That sounds fine to me, I imagine #1164 will be in testing much longer than this, so would be good to get this merged in.

[demiankatz]

Okay, merging this now! Thanks, @danihagosbl and @LlGC-jop!