NegiAkash890 / editor-frontend

Online Code Compiler Frontend Repository
https://editor.akashnegi.in/
17 stars 12 forks source link

Add Dark mode toggler #8

Closed arp99 closed 2 years ago

arp99 commented 2 years ago

Closes #6

capture-editor

arp99 commented 2 years ago

@NegiAkash890 Please review the PR

NegiAkash890 commented 2 years ago

Hi @arp99 I see you have done some good work but since we have two PRs using redux. I am actually thinking whether we can combine the redux together somehow. Tagging PR #7 . Let me know your view on same?

arp99 commented 2 years ago

Hi @arp99 I see you have done some good work but since we have two PRs using redux. I am actually thinking whether we can combine the redux together somehow. Tagging PR #7 . Let me know your view on same?

I have seen the PR using REDUX, but I do not think so you need to use REDUX for that small change, where you only keeping some boilerplate code, that's unnecessary, For smaller apps like this context does all the work and also updating everything on the global; store is not always necessary as it might make the app less performant

arp99 commented 2 years ago

And one more point both context and redux can work simultaneously, you can keep context for any part of the DOM tree

arp99 commented 2 years ago

Moreover the app currently lacks scoping in terms of CSS, it would be nicer we use CSS modules for each component, because keeping every class in the global namespace is not a good idea to scale

NegiAkash890 commented 2 years ago

Moreover the app currently lacks scoping in terms of CSS, it would be nicer we use CSS modules for each component, because keeping every class in the global namespace is not a good idea to scale

Can you resolve merge conflicts from your side?

arp99 commented 2 years ago

Moreover the app currently lacks scoping in terms of CSS, it would be nicer we use CSS modules for each component, because keeping every class in the global namespace is not a good idea to scale

Can you resolve merge conflicts from your side? More of such conflicts will happen in future when u merge PR s one by one

arp99 commented 2 years ago

Moreover the app currently lacks scoping in terms of CSS, it would be nicer we use CSS modules for each component, because keeping every class in the global namespace is not a good idea to scale

Can you resolve merge conflicts from your side?

Do not push the package-lock.json it will create more and more merge conflicts which is not easy to resolve

arp99 commented 2 years ago

Moreover the app currently lacks scoping in terms of CSS, it would be nicer we use CSS modules for each component, because keeping every class in the global namespace is not a good idea to scale

Can you resolve merge conflicts from your side?

Do not push the package-lock.json it will create more and more merge conflicts which is not easy to resolve

For the code you pushed to main, it is hard to resolve conflicts as you have changed lot of component definitions you have to resolve the conflicts now.

NegiAkash890 commented 2 years ago

Can u see screenshots of issue you are facing. Is it bcoz of linter? You can pull the main from the repo and install eslint. If you face any issue ping me?

arp99 commented 2 years ago

Can u see screenshots of issue you are facing. Is it bcoz of linter? You can pull the main from the repo and install eslint. If you face any issue ping me?

Yeah because of linter lots of files has changed, components with no body is directly returning the jsx wit brackets, making hard to edit for future, moreover the package-lock.json is huge to be edited, do not push package-lock.json

arp99 commented 2 years ago

Refactor your code to use css modules that would not cause much problems

arp99 commented 2 years ago

I am done with merging but there will be conflict with package-lock.json as It is hard to resolve

NegiAkash890 commented 2 years ago

I am done with merging but there will be conflict with package-lock.json as It is hard to resolve

Why package-lock.json is necessary? Please read this https://nodejs.dev/learn/the-package-lock-json-file

NegiAkash890 commented 2 years ago

Can u see screenshots of issue you are facing. Is it bcoz of linter? You can pull the main from the repo and install eslint. If you face any issue ping me?

Yeah because of linter lots of files has changed, components with no body is directly returning the jsx wit brackets, making hard to edit for future, moreover the package-lock.json is huge to be edited, do not push package-lock.json

Yup linters are difficult to setup and understand but it's necessary for the project at this time. I am following AIRBNB guidelines I think it will not be an issue in future. If it's difficult for you. I can resolve the conflicts no worries!