Closed fmechant closed 3 years ago
Hi Pedro,
Thank you for the review.
Although I think you are optimizing prematurely, I do agree with some of your changes.
But, doing these changes increases the scope of this fix, and I am not sure I like that.
Maybe others can share their thoughts about this?
I simplified the updateKeyDown. Not sure if that is OK. Please let me know.
I want to add a unit test for findFocusedItem, and even for update with a OnKeyDown message, but there are no tests, at the moment. Do you want me to add these tests?
Disadvantage of testing findFocusedItem directly, is that it needs to be exposed. I guess we do not want that?
If we also do not want to expose Msg(..), than testing the update becomes impossible, I think.
I do not have experience with elm-program-test
, but I am willing to spend some time implementing it for using the keys.
Sounds like an interesting idea, no?
I do not have experience with
elm-program-test
, but I am willing to spend some time implementing it for using the keys. Sounds like an interesting idea, no?
Yeah, this is really interesting. But, one thing I noticed is that we're missing the ARIA roles, and that would be essential to finding the proper element "to click" in the E2E tests. This example has an adequate implementation of it.
If you want, feel free to open another issue and PR to track it, I'll add it to our CI for proper checking on every merge.
Just a warning, porting to elm-program-test
is a little more complicated than it looks. But it's good to learn it for someone who works (or wants to work) with Elm.
Filtering items when looking for which item is focused, so that the correct item is selected when using the arrow keys and enter key.