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

Adds an "open" stateChangeType and sets that state on open. #715

Conversation

claytron5000
Copy link

What:
Adding a new open state change, and setting that on menu open.

Why:
this issue: #708

How:
Added an exported const to the StateChangeTypes, assigned those on open.

Checklist:

  • Documentation
  • Tests
  • Ready to be merged
  • Added myself to contributors table

This is not ready for merge. I'm having trouble running the startup script. I've opened an issue for that here #712, I'm hoping that gets moving so that I can properly run tests/compilation for this PR.

@silviuaavram
Copy link
Collaborator

I am pretty confused why this would work. Does it open on the first item when you open it by Arrow Down? Does it open on the last item when you open it by Arrow Up?

@claytron5000
Copy link
Author

The previous code was a little confusing. I'll start by outlining what was happening, then I think my code changes become clearer.

On arrow down call to internalSetState sets isOpen to true, and sets the type to keyDownArrowDown. The callback that follows calls the setHighlightedIndex method with the type again set to keyDownArrowDown. This results in two state changes, both attributed to the keyDownArrowDown.

On arrow up call to internalSetState sets isOpen to true, and sets the type to keyDownArrowUp. This is followed by the callback, which mistakenly sets the type to keyDownArrowDown. This results in two opposite state changes, up and down.

I'm adding the open state change with the following thought:
The initial set state method is opening the drop-down, and so that state change type really is "open(ing)". The callback moves the highlighted index, which will either be up or down.

Looking at this now, the bare minimum is to fix that second call the keyDownArrowDown in the down arrow callback. I do think there is a good reason to set two different state change types, one for opening the drop-down, one for moving the highlight.

@silviuaavram
Copy link
Collaborator

Coming back to this. I think the problem is my error in this PR and it should be fixed by replacing ArrowDown with ArrowUp in the second call.

It will still return twice in the reducer unfortunately but with the same key this time (2xArrowDown or 2xArrowUp) because we get the items after render and then we need to set state again (re-render) with the index.

Can you just change that in this PR or should I create one and ask your review?

@claytron5000
Copy link
Author

I think there could be value in adding an "open" stateChange type, but I can see why it doesn't belong here. I'd be happy to review if you make the change. I still don't have the pre-commit hook running, so I have to hack through to get code up.

@silviuaavram
Copy link
Collaborator

Closing this, I don't want to change this behavior in Downshift. For a one-state change including open+highlight please move to useSelect or useCombobox. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants