Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add bttv/ffz emote support #59

Merged
merged 8 commits into from Jul 21, 2021
Merged

Conversation

juaoose
Copy link
Collaborator

@juaoose juaoose commented Jul 18, 2021

This adds support for BTTV emotes for chat history.

  • I am not conviced the current parsing is very efficient?
  • I used the mechanism you used for badges, does it apply
  • Would FFZ ones fit in the same model (renamed) or would it be better to have another ChangeNotifier for FFZ?

@juaoose juaoose marked this pull request as draft July 18, 2021 03:24
@juaoose juaoose marked this pull request as ready for review July 18, 2021 03:33
@kevmo314
Copy link
Contributor

kevmo314 commented Jul 18, 2021

I am not conviced the current parsing is very efficient?

Yeah the rest of the PR looks good, I'll leave a few comments on the parsing to improve that.

I used the mechanism you used for badges, does it apply

Yes this is fine :)

Would FFZ ones fit in the same model (renamed) or would it be better to have another ChangeNotifier for FFZ?

I think putting them in the same model is fine. We can call it ThirdPartyEmotesModel or something.

@kevmo314 kevmo314 linked an issue Jul 18, 2021 that may be closed by this pull request
@juaoose juaoose changed the title feat: add bttv emote support feat: add bttv/ffz emote support Jul 19, 2021
@juaoose juaoose requested a review from kevmo314 July 21, 2021 02:47
Copy link
Contributor

@kevmo314 kevmo314 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks good :) I think we should refactor the processing into the model in the future bc there's some benefits to that for future features, but that can be done in a separate PR. I'll take a look after this one gets in.

Nice work!

@juaoose juaoose enabled auto-merge (squash) July 21, 2021 02:54
@juaoose juaoose merged commit 1bc3a3b into muxable:main Jul 21, 2021
@juaoose
Copy link
Collaborator Author

juaoose commented Jul 21, 2021

Code looks good :) I think we should refactor the processing into the model in the future bc there's some benefits to that for future features, but that can be done in a separate PR. I'll take a look after this one gets in.

Nice work!

I actually asked about the function location because I was thinking on how to test it compared to the other parsing functions in this file. And there is some refactoring to be made to integrate these to the emote picker too.

@kevmo314
Copy link
Contributor

I actually asked about the function location because I was thinking on how to test it compared to the other parsing functions in this file. And there is some refactoring to be made to integrate these to the emote picker too.

Ah yeah, I think testing becomes easier if this complex logic is refactored into the model. But y'know, merge first think later. Lots of people will be very happy with BTTV support :)

@juaoose juaoose deleted the bttv-support branch July 21, 2021 03:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[chat_history] BTTV/FFZ emotes
2 participants