Closed tolgap closed 8 years ago
Thanks! I added some inline comments.
@pencil Thanks for the review, I applied your suggestions. Would you like me to squash the commits into a single one?
Looks good. What browser(s) did you use to test the change?
@pencil I had tested on Chrome and Firefox. I'll get on it to patch the $.ajax
for Safari and test it on that as well. I have some time tonight.
@pencil I have rewritten the $.ajax
call as a ReChat.get
call. It works on Chrome and Firefox.
I tried to install the extension on Safari but you need to be an Apple Developer, and that means paying €99 for me. I'm not gonna do that. Is it possible for you to review for Safari?
Seems to work in Safari :+1: @goldbattle mentioned a bug when using BTTV. Would be pretty good if we could address it as BTTV users have been pretty vocal (reviews of ReChat, posts in subreddit) whenever there was an incompatibility.
@pencil I installed BTTV, and indeed it does mess up the badges. The thing is... they use the !important
css property all over the place... And overriding that is a bitch and a half because ReChat does not inject its own CSS. So I had to resort to fixing it using jQuery, and it's not really nice...but it gets the job done.
Here's with BTTV:
Here's normal ReChat:
You also notice a difference, as BTTV increases font size from 12px
to 13.33333px
(for some reason)
Seems to work:
Only problem is with split chat the emotes seem to get cut off for some reason, and sometimes the sub icon is too large so the second line of text does not make it all the way. Probably changing the line height would fix that. Good job!
I turned on split mode, and that sets line-height: 16px!important;
. After changing that to line-height: 20px!important
, it looks like this:
I'll commit this as well. Would like a review of the last commit :+1:
That did not seem to work for me, nothing was affected. But when I apply the line height to the message span it seems to work as expected.
message = $('<span>').addClass('message').css({ 'word-wrap': 'break-word' }).attr('style', 'line-height: 20px!important;'); // BTTV style overriding
@goldbattle I've moved it to the .message
div. It seems if a chat-line included multi-line text+emotes, it changed behavior. This is how it looks now:
I am not sure why it is preforming differently. I am using chrome 46.0.2490.13 beta-m (64-bit).
On Sun, Sep 6, 2015 at 9:31 AM, Tolga Paksoy notifications@github.com wrote:
@goldbattle https://github.com/goldbattle I've moved it to the .message div. It seems if a chat-line included multi-line text+emotes, it changed behavior. This is how it looks now:
[image: screen shot 2015-09-06 at 15 29 32] https://cloud.githubusercontent.com/assets/1763486/9704558/4eb73654-54ac-11e5-836e-4aeb87e6b607.png
— Reply to this email directly or view it on GitHub https://github.com/pencil/rechat/pull/26#issuecomment-138086053.
@goldbattle can you send me the VOD+timestamp which you're testing with?
I have been watching this one: http://www.twitch.tv/lirik/v/14393772 From the beginning.
@goldbattle I'm using Chrome 45 stable 64bit on Mac OS X. This is how "OmairTitan" his message looks:
Is it really different from this?
Works great.
@goldbattle Yeah you still have the issue of misaligned badges it seems. Unless that was sarcasm :shipit: . I'm going to adjust my screen resolution and play around with that, as yours seem to be 1080p, whereas mine is 1440p with a large PPI. It might affect the positioning of emotes.
It is better then before, I didn't mean sarcasm, but yes the emote alignment is still an interesting bug.
@goldbattle I just cannot reproduce that man. Tried playing around with my resolution and DPI, but the badges align correctly. Could it really be Chrome beta 46 messing something up?
@pencil Could you try it out as well and check if the misalignment happens for you as well? Thanks!
I am on Chrome 46 too and this is what it looks like:
Edit: Forgot to rerun setup.sh
. Updated Screenshot.
@pencil @goldbattle I have removed most custom styling. Only fixed the float issue on badges. It still looks good, and now all styling is done by either Twitch's CSS, or BTTV's CSS. I think this is the go-to solution. Can the last commit be reviewed?
With the new version, emotes on the first line seem to cause the name to be misaligned.
Is either of you guys still working on this? I really would like to see this feature in the next version but I probably will not have time to fix it since I have to work on some server side issues.
I can't devote any time to this as I have exams, the float issue is such a pain.
On Thu, Oct 8, 2015 at 12:19 AM, Nils Caspar notifications@github.com wrote:
Is either of you guys still working on this? I really would like to see this feature in the next version but I probably will not have time to fix it since I have to work on some server side issues.
— Reply to this email directly or view it on GitHub https://github.com/pencil/rechat/pull/26#issuecomment-146415322.
@pencil The feature on itself works, but working around BTTV's split messages option... I simply fail at it.
If there are beta releases of ReChat, we could simply merge the feature, while not being compatible with BTTV. And before actually releasing the chat badges feature, have someone else/us take another look at it.
At the moment, I am absolutely flooded with work from my full time job.
@tolgap I really wish you would have tagged anyone on the BetterTTV team instead of working around and complaining about our CSS. I submitted a PR to your fork, and pushed changes to our repo to fix the formatting issues. Please don't hesitate to reach out for further issues.
Thanks @night! Really appreciate it!
@night Thank you!
I didn't even know BTTV was on Github! Just to be clear, I never wanted to complain about the CSS of BTTV. I wanted to complain about how I fail at CSS and how hard I think is overriding !important
tags. I should have phrased it better.
Looks like we have conflicts. Can you please merge master
into your branch and resolve them?
@pencil Merged master
into my branch and resolved the conflicts.
Just tested this, seems to work great, both with the added bttv emotes, and the chat tags. Thank you again @tolgap and @night !
@pencil bump?
Sorry for the delay guys.
I currently don't have time for extensive testing which is why I will do a gradual rollout of version 0.19.0 that includes this change. Currently 10% of Chrome users should get the updated version and I will crank up this number if there are no bug reports.
Shoutout to @goldbattle for providing some code to start this PR. See discussion in issue #20 on why I opened a new PR for this.
This PR introduces chat badges for Streamer, Staff, Admin, Global mod, Mod, Turbo and Subscribers.
It checks for the following properties of
messageData
:from
matchesReChat.Playback.channelName
=>broadcaster
usertype
matches one ofmod
,global_mod
,admin
,staff
turbo
istrue
subscriber
istrue
The badges are constructed using
ReChat.Playback._buildBadge
so if badge styling needs to be updated, only_buildBadge
needs to be edited.