no-stack-dub-sack / cs-playground-react

In-Browser Algorithm and Data Structures Practice
http://cs-playground-react.surge.sh/
MIT License
520 stars 91 forks source link

Feat/isaac abrahamson ia menu #56

Closed no-stack-dub-sack closed 6 years ago

no-stack-dub-sack commented 6 years ago

@IsaacAbrahamson, I'm going to try to figure out how to push commits directly on to your other PR (I know one way, but its a bit of a pain), but this way still preserves all your commits and gives you the credit.

Changes:

I think this is it, but maybe one or 2 other things, with the note above and maybe one or two more fixes done, I think this will be all set. How are you liking it?! Here's a preview: http://questionable-number.surge.sh/

IsaacAbrahamson commented 6 years ago

LGTM 👍

Yea, see if you can put that on mine, that would be really nice 😃

I'll see if there is another way to have icons without svg. Out of curiosity, would it work if we wrapped the icons in a container?

no-stack-dub-sack commented 6 years ago

Out of curiosity, would it work if we wrapped the icons in a container?

@IsaacAbrahamson No, I don't think so, the end result would be the same: a bunch of nested elements that can be clicked on individually. What's happening with the book one, I think, is that if you click like right on the top inside corner (fully inside the line in the blank space), you're not actually clicking on the SVG element at all, and just clicking through to the underlying menu item. Hmm... now talking it out actually, that may help. If the SVG element is wrapped in a container, it should prevent this (if this is actually what's happening) — when you click through, the container would be underneath, not the menu item, and e.currentTarget.id should correctly fire the event in any case. Def worth trying!

Yea, see if you can put that on mine, that would be really nice

I will try, I should have just merged yours first, and then made changes, but when I pulled it down I made one change, then another, and before I knew it it was getting tough to go back. I do think the commit history would look exactly the same even if this PR were merged though, as you can see above, it still recognizes all of your commits. I'll see if I can fix it later though. If I merge your PR, this one may still work.

no-stack-dub-sack commented 6 years ago

@IsaacAbrahamson Ok, this is much cleaner now. I merged in your original (2nd PR), and then resolved the merge conflicts here since this one originally had both sets of changes. And now the diff here only shows the changes I've made on top of yours, instead of both sets, which made it much harder to parse what was mine and what was yours.

So you get full credit for the last PR, and it looks like you get double credit lol, because your commits still show here as well, so the commit history will show the same commits twice (my fault). Whoops haha :smile:

no-stack-dub-sack commented 6 years ago

Out of curiosity, would it work if we wrapped the icons in a container?

Ended up trying it... didn't work. Hmm... stumped. Not that I've tried much else, but you have to try pretty hard for it not to work, so may just leave it.

IsaacAbrahamson commented 6 years ago

So you get full credit for the last PR, and it looks like you get double credit lol, because your commits still show here as well, so the commit history will show the same commits twice (my fault). Whoops haha 😄

Haha, that's a good one.

Ended up trying it... didn't work. Hmm... stumped. Not that I've tried much else, but you have to try pretty hard for it not to work, so may just leave it.

I'll take a look at adding another type of icons.

no-stack-dub-sack commented 6 years ago

@IsaacAbrahamson

I'll take a look at adding another type of icons.

No worries, and no need, sorry, didn't see this sooner. It seems like all the major icon services have moved to SVG anyway (FontAwesome apparently recently changed over), so I came up with another fix which you can see here in the Modal.js file on the closeModal and targetContainsModalTrigger methods. There's a comment there which explains what was going on (related to SVG nesting, but slightly different problem than before).