mlomb / chat-analytics

Generate interactive, beautiful and insightful chat analysis reports
https://chatanalytics.app
GNU Affero General Public License v3.0
711 stars 51 forks source link

Support for Guilds (and multiple of them) #39

Closed mlomb closed 1 year ago

mlomb commented 1 year ago

We would like to store guild data from Discord exports and include the guild concept in other platforms

Should fix #9 and then we can work in #12.

cloudflare-workers-and-pages[bot] commented 1 year ago

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: cee3bfa
Status: ✅  Deploy successful!
Preview URL: https://0ec4839b.chat-analytics.pages.dev
Branch Preview URL: https://guilds-support.chat-analytics.pages.dev

View logs

hopperelec commented 1 year ago

I know this hasn't been merged yet, so some of these will already be planned, but just in case some aren't I'll list the things I've noticed so far that might be worth doing:

hopperelec commented 1 year ago

Why is twoOrMoreGuildsPresent not just integrated directly in the if expression? Is it going to be referenced elsewhere? https://github.com/mlomb/chat-analytics/pull/39/commits/8dd6306e1299d7624fa092b770afe3977b7bad5d#diff-17ea9fcb0494c19a66704fa349d7d08f2f59ac34e3f5a87d4b035ad44143d763R26-R36

mlomb commented 1 year ago

Why is twoOrMoreGuildsPresent not just integrated directly in the if expression? Is it going to be referenced elsewhere? 8dd6306#diff-17ea9fcb0494c19a66704fa349d7d08f2f59ac34e3f5a87d4b035ad44143d763R26-R36

I think it's clearer. That specific check will probably change later tho

Edit: I changed my mind

mlomb commented 1 year ago

I think we are ready to merge this!

hopperelec commented 1 year ago

Not sure if this should be part of this PR or if it should be done separately afterwards, but I think there's been enough significant changes at this point that the demo should really be updated. I've mentioned allowing contributors to update it in issue https://github.com/mlomb/chat-analytics/issues/31 but until such a thing exists it should be updated to at least reflect most of the recent major changes

mlomb commented 1 year ago

I know, but I'll wait a bit longer to update it. Ideally it should be generated at build time

I've tested this PR with a lot of combinations and everything was right, I think it is time to merge. The only thing remaining would be to fit the characters correctly in TextAvatar, but I will leave it as is for now.