search

swarmcity / terminal-ts

Terminal implementation in TS
0 stars 0 forks source link

feat: implement the new design #30

Closed vojtechsimetka closed 2 years ago

github-actions[bot] commented 2 years ago

🐝 PR preview in Swarm

Preview URL: https://bah5acgzawd7ypcquxx3cg7wlpfcqgts4ov6bg4wdggkl3o76hg5arh7hu3kq.bzz.link Swarm Hash: b0ff878a14bdf6237ecb7945034e5c757c1372c33194bdbbfe39ba089fe7a6d5 Commit Hash: d05dca88d52aebd7a88f4c09e4192bfde4013355 Commit Message: chore: run prettier

vojtechsimetka commented 2 years ago

Thanks for the review, here are few comments, but happy to further discuss on a call.

  • I think we should try to organize imports a bit more (external libraries vs local components vs assets etc).

I really think this would be a waste of time without much of an upside. If anything we can introduce linter rules that automatically organizes it alphabetically (well first it imports modules and then local files). I can't see a word, where some further specific organization improves readability. This may be skewed by the way I am developing, but if I need to find specific import I just click on the symbol in UI.

  • I think we should be consistent with async/await instead of using Promises, although I realize that it's not always practical with React.

I tend to agree, but there are places where you can't or should not use await. E.g. event handlers can not be async functions.

  • I'm not sure prettier ran everywhere on here, but eslint doesn't seem to complain, so should be fine?

I run prettier each time before I commit usually. So should be fine :)

  • Remove the unnecessary assets (qrCode.svg, all checkmarks except the blue one, ...)

OK, I guess this is on me for not finishing the whole flow in this PR.

vojtechsimetka commented 2 years ago

Oh yeah, the prettier was not being run... We should add something like "prettier": "prettier . --ext .ts,.tsx --write" script

filoozom commented 2 years ago

Oh yeah, the prettier was not being run... We should add something like "prettier": "prettier . --ext .ts,.tsx --write" script

We absolutely can. I get why it didn't run on .webmanifest and .html (fixed in #37), but it should have worked for the .html...