Skip to content

Fix li click event #12

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

MichaelWashburnJr
Copy link

@MichaelWashburnJr MichaelWashburnJr commented Nov 16, 2016

@hannaholl I'm making separate PRs for these changes per your request in #8. [Edit] I just squashed these commits also

Previously, I couldn't click a list item with my mouse. This patch
lets the user click the list item to select it.

Remove check for equal value

This was stopping the initially selected value from being set

Fix selecting on mouse enter

remove console log
@hannaholl
Copy link
Owner

Thank you! :)

Sorry, another thing now.
Here I think we should keep ng-mouseenter="setActive($index)" so that on desktop you can still mouse over an item to make it "active"?

Also, can you please add this if and else back in?

Just add more commits here without squashing if that helps. And after that we can look at merging the placeholder too. Thanks!

@MichaelWashburnJr
Copy link
Author

@hannaholl that if/else block prevented the initial value from being rendered in the browser for one reason or another. It's really not needed, because it only saves you 3 operations if the process enteres the else instead of the if.

Regarding ng-mouseenter, this selected an item when the user hovered over it for me. Is that the intended functionality? because it made using it very difficult. As far as styling the item goes, I think the CSS :hover is a better option.

@hannaholl
Copy link
Owner

Hm, how did you set the initial value when it wasn't rendering? It's been a while since I worked on this project so I don't actually remember everything about how it works.. But I'm sure that if was there for some reason, maybe to fix some edge case. I'm aware the else doesn't do anything, but I left it there in case I (or someone else using this) want to change the behaviour in the future.

The mouse-enter should be setting an item as active, so that when you press return that item get selected. As you can see it's calling setActive: ng-mouseenter="setActive($index)" and not setSelected(). Are you sure it selected the item for you?

It's fine if this all works in your implementation, but I can't merge these changes as they might add issues for other users. If you can provide an example on codepen or something I can check that everything is working as intended when I get some time. Cheers!

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