ninest / Shots

🥃 A party drinking game that lets you learn more about your friends: Provider + Hive + swipeable_card
https://shotsapp.now.sh/
GNU General Public License v3.0
134 stars 20 forks source link

refactored #8

Closed misterfourtytwo closed 4 years ago

misterfourtytwo commented 4 years ago

seen ur project on reddit

made logic cleaner and less error-prone ui is somewhat flexible and scales lil bit added sounds in web and android, app now also works on web and linux, should also work on other desktop systems, though haven't checked forked before you started using your package, imo it's easier for u to edit merge so it will work with it

checked on android, web and linux hosted on pages

sounds and colors and styles still need some changes though

ninest commented 4 years ago

Thank you so much for taking the time and effort to go through the project and make changes. I haven't gone through everything, but here are some initial comments.

Additionally, what do you mean by "less error-prone"?

Thanks for the PR

Added later:

misterfourtytwo commented 4 years ago

const is preset uncahangable value, while getter is a function call, which introduces overhead. audio works on my phone i just added random shuffle noise from web map is usual method for mapping iterables, for loop might have worked, but for cards to update properly they have to get unique keys for them, which is why i use indexOf, if i just used index from for loop framework will just reuse swiped away topcard again and introduce bugs with update i thought reshuffle button meant for restart, not just for the onscreen deck reset when i debugged on my monitor colors were really eye gouging, so i changed them a bit. also, blue ones was too dark in comparison to other cards. so that scaffolding will be written in the same uniform way db is folder generated by hive when run on my desktop, shoud've removed it, sorry

ninest commented 4 years ago

audio works on my phone

Thanks for testing!

when i debugged on my monitor colors were really eye gouging, so i changed them a bit. also, blue ones was too dark in comparison to other cards.

Thanks for informing me, I'll look into it

One more thing, you were doing something involving scaling. What exactly were you doing? Trying to make the card sizes responsive to screen sizes?

map is usual method for mapping iterables, for loop might have worked, but for cards to update properly they have to get unique keys for them, which is why i use indexOf, if i just used index from for loop framework will just reuse swiped away topcard again and introduce bugs with update

Is this the change you were referring to when you said "made logic cleaner and less error-prone"

misterfourtytwo commented 4 years ago

yeah, before, when card container was too big, it was moving only in one axis, or movement weren't visible at all. also added layout rebuilds in main screen menu, before when switched to landscape some of the widgets were rendered off-screen

ninest commented 4 years ago

I'm sorry, but I don't think I'll merge this pull request. There are too many changes. The new sound you added is nice, but there's too much I'll have to roll back. Additionally, I haven't planned web support yet as there are still some issues on Android and iOS I am yet to fix.

Next time please make smaller PRs after discussing by creating issues. Thank you so much for your efforts!

misterfourtytwo commented 4 years ago

well, i did it mostly for myself. i think i might implement other things later, like creating custom packs, feel free to watch my fork.

ninest commented 4 years ago

Sure, thanks for understanding!