fix: pickListItem. spacebar on secondaryAction no longer toggles selection

[pr3tori4n]

Related Issue: #309

Summary

pressing spacebar while the secondary action is focused no longer checks/unchecks the list item.

[driskull]

I think there are a couple issues with the structure here. The onclick for the secondaryAction should be a <button> element so that it supports spacebar/enter key to trigger it.

What might be better to fix this issue is to move the onclick from the Host to the <label> Then there are no longer any nested "buttons".

[pr3tori4n]

Moving the onclick to the label would severely reduce the hit area. I don't think that's a valid option.

There something wrong with this approach?

[driskull]

Moving the onclick to the label would severely reduce the hit area. I don't think that's a valid option.

That can be addressed by styling. (Moving the padding from the host to the label).

The issue is similar to what we have in the API layerlist widget. The hit area shouldn't be the whole item, just the label and checkbox. Then you won't have nested buttons or have to prevent the internal click from bubbling.

[jcfranco]

Good to merge once @driskull's feedback is addressed.

[pr3tori4n]

@driskull I made the updates you suggested, however I still had to keep the onkeydown event on the host because I was getting a typescript error when I tried putting it on the label. Therefore I also had to keep that condition in the event handler.

[driskull]

Ok.

onkeydown event on the host because I was getting a typescript error when I tried putting it on the label.

I ran into that issue before and used a onkeyup instead. I'm not sure if typescript allows for putting an onkeydown on a div.

[jcfranco]

That's weird. I don't see why it would be typed to allow one vs the other. Maybe it's a Stencil bug.

[driskull]

yeah. I'm not sure either. Maybe some investigation or bug issue for them.

@pr3tori4n would onKeyUp work for the use case?

[driskull]

Can you try moving the onKeyDown to the <label> element. I think an update might have fixed that bug. 👍

Good to merge once conflicts resolved.

[pr3tori4n]

neither onKeydown or onKeyup is passing typescript validation on the label element.

@driskull how would you like to proceed?

[driskull]

neither onKeydown or onKeyup is passing typescript validation on the label element.

Checked out local branch and this seems to work:

<label class={CSS.label} onClick={this.pickListClickHandler} onKeyUp={this.pickListKeyDownHandler}>

[driskull]

@pr3tori4n Also seeing this:

The JSDoc errors

[driskull]

We can merge this in and have a followup issue for the key handler. Lets fix the jsdoc though.

[pr3tori4n]

It might be being fickle on the casing. I was using onKeydown and onKeyup not onKeyUp. Follow up issue sounds good tho.

[driskull]

Created issue for the handler. #433

I think we just need to delete the jsdoc for anything with @property and @type as it is unnecessary.

[jcfranco]

Looking good.

[jcfranco]

It might be being fickle on the casing. I was using onKeydown and onKeyup not onKeyUp.

It's not being fickle, it's being correct. Look at dist/types/stencil.core.d.ts for reference.

[jcfranco]

👍

[jcfranco]

[Nitpick] We should be using tabIndex, no? We can do inventory on this in a follow-up issue.

[driskull]

Yep. fixed! only found a couple