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

Add chat badges to HTML downloads #564

Merged
merged 3 commits into from Mar 5, 2023
Merged

Add chat badges to HTML downloads #564

merged 3 commits into from Mar 5, 2023

Conversation

yann-a
Copy link
Contributor

@yann-a yann-a commented Mar 1, 2023

Hi, this is my attempt at adding chat badges to HTML downloads.

@yann-a
Copy link
Contributor Author

yann-a commented Mar 1, 2023

Some parts I'm a bit unsure of:

  • When embedData is true, it should probably create a dictionnary of (_id, version) to badge instead of iterating over chatRoot.embeddedData.twitchBadges for every chat badge, but it's unclear for me why chatRoot.embeddedData.twitchBadges isn't already a dictionnary
  • I think creating BadgeResponse.cs was necessary? It's kind of a duplicate of Api/TwitchBadgeResponse.cs, but it's not in the scope of the api. Also, I'm not sure of my class names there.

TwitchDownloaderCore/Chat/ChatHtml.cs Outdated Show resolved Hide resolved
TwitchDownloaderCore/TwitchHelper.cs Outdated Show resolved Hide resolved
TwitchDownloaderCore/Chat/ChatHtml.cs Show resolved Hide resolved
@ScrubN
Copy link
Collaborator

ScrubN commented Mar 3, 2023

but it's unclear for me why chatRoot.embeddedData.twitchBadges isn't already a dictionary

Because backwards compatibility

@yann-a
Copy link
Contributor Author

yann-a commented Mar 5, 2023

I updated the PR to use EmbedChatBadge and factor the GetChatBadgesData code. I rebased on your upstream changes because they were conflicting.

@ScrubN
Copy link
Collaborator

ScrubN commented Mar 5, 2023

Looks awesome, and chat JSONs are unaffected aside from the empty URL list. Thanks for the PR!

Also I noticed that there is a scaling issue with first party emotes. I'll fix that once this is merged.

Co-authored-by: Scrub <72096833+ScrubN@users.noreply.github.com>
@ScrubN ScrubN merged commit 650e468 into lay295:master Mar 5, 2023
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.

None yet

2 participants