SidneyNemzer / snippets

A Chrome extension that allows you to create and edit JavaScript code snippets, which are synced to all your computers
https://chrome.google.com/webstore/detail/snippets/fakjeijchchmicjllnabpdkclfkpbiag
63 stars 9 forks source link

fixed issue when clicking MoreVert #12

Closed nionis closed 7 years ago

nionis commented 7 years ago

Slightly annoying tiny thingy

SidneyNemzer commented 7 years ago

Hi! Thanks for the pull request. Could you clarify what issue this solving? Based on the code, it selects a snippet when you hit the menu icon (the three dots), instead of JUST opening the menu.

Actually, I intentionally don't have it select the snippet when you hit the menu icon. Based on this pull request, you don't like that behavior. Which I appreciate! 😉

Is that right?

nionis commented 7 years ago

The reason why I think it should select the snippet its because u could have selected item A but then pressed three dots of item B, the menu of item B will show but item A will remain selected, in my opinion it should select item B since its the menu u are viewing. Tiny bit detail that doesnt matter that much, its up to ur preference I suppose

SidneyNemzer commented 7 years ago

That makes sense, I'm OK with changing the behavior of the menu button.

I've got a question regarding the code now: couldn't you just do this?

event => {
  if (!selected) {
    this.props.selectSnippet()
  }

  this.handleMenuOpen(event)
}

Maybe I'm missing something obvious, but I don't see a benefit to Promises in this case

nionis commented 7 years ago

I thought setState's async functionality would cause an issue but it seems it doesnt (rusty react skills) I have made the change and put it in one commit

SidneyNemzer commented 7 years ago

Ah, I see. The promise that selectSnippet() returns is only used when deleting a snippet, since the order of actions does matter in that case (we need to deselect the snippet before deleting it!). In this case, it doesn't matter what happens first, the menu opening or the selection changing.

Looks good, I'll merge this today or tomorrow and update the version on the chrome store

nionis commented 7 years ago

perhaps u should wait before updating the chrome store since I will be making another pull request with a basic version of the autorun feature soon

SidneyNemzer commented 7 years ago

Fair enough, I'll wait on updating the Store version.

(Didn't have a chance to merge this last night. I'll do it tonight)