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

Keep the menu state #66

Closed mansisce closed 6 years ago

mansisce commented 6 years ago

track menu state Please review and provide your inputs. There are few things which could be done in a better way. I will be refactoring that.

no-stack-dub-sack commented 6 years ago

@mansisce At first glance, this looks great! What do you have in mind as far as refactoring? I have a couple of comments so far, but mostly related to style (I will eventually be adding a lint pipeline to enforce this, but I'm not too concerned about it just yet).

Once you add a reducer to handle the logic and the toggleMenu action, I think this will be in pretty good shape and I can give it a more careful review before we merge it in.

no-stack-dub-sack commented 6 years ago

@mansisce the changes look good to me, thanks! I think the code is much more readable now with more descriptive names. Do you feel comfortable with Redux to add the reducer logic?

mansisce commented 6 years ago

@no-stack-dub-sack Ooops i missed commiting those files. Ofcourse I'm comfortable with reducers and action creators. I've added them please have a look.

no-stack-dub-sack commented 6 years ago

@mansisce Ok, haha 😆 This makes much more sense now! I was wondering how you got that far without adding reducer logic and action creators :smile:

mansisce commented 6 years ago

Please have a look now. Should be working for you now.

mansisce commented 6 years ago

Sorry for buggy code commit

no-stack-dub-sack commented 6 years ago

@mansisce Awesome! Great work with this! I'm going to merge this into staging, because I have another big PR coming soon, and I want to get these changes merged in with mine before merging everything into master. Once I merge in my changes, I may make some small refactors that will help this work with an additional menu item I'm adding that will be coming from a different component.

If you're interested, check out the new feature deployment here: https://questionable-number.surge.sh/

In addition to the ability for users to add their own "files" or repls now (see the bottom menu item), I've also built an API that allows for users to share code in case they ever want someone else to take a look at what they're working on. Follow this link to see what I mean: https://questionable-number.surge.sh/share-repl/5a989a0df159bd0014f46150

Let me know what you think of all this! Eventually, I will prob need to add the ability to register so everything doesn't have to be managed by local storage.

Please let me know if you're interested in contributing any more. One other thing we really need is for more algorithms to be added to the project. It's less fun than contributing new features, but I think its what will really help make this app better for users. The more content, the better :smile:

Thanks again for your help, and great job once again!

mansisce commented 6 years ago

@no-stack-dub-sack Inlining my comments : Thanks for looking into my PR and merging the same into staging.

Once I merge in my changes, I may make some small refactors that will help this work with an additional menu item I'm adding that will be coming from a different component.

Yes I do aware of this existing issue with the current implementation. It is not handling the nested menu scenario. Also, we have hardcoded menu currently with details tag in Menu.js and MenuMap.js. Ideally in CODE we should have nested menu, so that we should put Easy and Moderate inside the Algorithm Challenges array. There is scope of refactoring there. What do you suggest on this. If we address this issue then you might not have to handle this everytime there is a new menu item.

If you're interested, check out the new feature deployment here: https://questionable-number.surge.sh/ This looks interesting.

In addition to the ability for users to add their own "files" or repls now (see the bottom menu item), I've also built an API that allows for users to share code in case they ever want someone else to take a look at what they're working on. Follow this link to see what I mean: https://questionable-number.surge.sh/share-repl/5a989a0df159bd0014f46150 Another cool stuff. How did you achieved so far.

Let me know what you think of all this! Eventually, I will prob need to add the ability to register so everything doesn't have to be managed by local storage. What do you mean by register.

Please let me know if you're interested in contributing any more. Yes ofcourse I'm interested to contribute more. Let me know what can I take next.

One other thing we really need is for more algorithms to be added to the project. It's less fun than contributing new features, but I think its what will really help make this app better for users. The more content, the better 😄 Yes I'll try to do that :)

no-stack-dub-sack commented 6 years ago

@mansisce sorry for the late reply! Didn't see the notification in my email and I stopped monitoring the PR as closely once it was closed.

Ideally in CODE we should have nested menu, so that we should put Easy and Moderate inside the Algorithm Challenges array.

Yes, this is a good suggestion, and I think would lend towards making the codebase a bit cleaner, but it would also complicate the reducer logic a bit since we couldn't do just a once over iteration of CODE anymore, since we'd have to go down one level to get into Algorithm Challenges. I may not be explaining this properly, but I think you see what I mean.

In the staging branch, there is also a new component for the "Repls" menu item, since the added logic of adding "new" files complicated things. But I think there's probably a better and more functional way to do this that would allow better reusability of components. Some refactoring here to look into.

I also made the change in the staging branch to handle saving state of the outer menu item.

Another cool stuff. How did you achieved so far.

In addition to this new component, I added some logic in App.js and Controls.js to make and handle the API call, which is a separate repo found at the link below. This is just a simple Node server with a MongoDB backend. It's a REST api which handles the logic and actions for storing and retrieving code strings. If an share link is detected, app.js slices off the mongo id and retrieves the code. You can see that here and here. Here's the API repo: https://github.com/no-stack-dub-sack/cs-pg-react-api

The staging branch is just about ready to be merged into master and deployed, so your changes will be live soon!

What do you mean by register.

I mean, I'd eventually like users to be able to register / sign up, so that we can store their code in a database, so they could access the site from a different computer and still have their saved state instead of just relying on local storage. But this is a big change, and not sure if I'm ready to make it yet. What do you think? Would this be better?

Let me know what can I take next.

There's a few open issues, but I'm open to suggestions as well. Do you have any thoughts on other refactors to the codebase? If so, feel free to open an issue!

Also, just curious - how did you hear about cs-playground-react? Did you read my Medium article, or somewhere else? I'd love to continue making this site better and get even more users!