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

Tcrd foreground fix #136

Merged
merged 6 commits into from
Aug 19, 2018

Conversation

AdamSmasherDerby
Copy link
Contributor

Updates the TCRD overlay and whiteboard screens so that the foreground text color uses the "overlay_fg" color.

if (match == null || match.length == 0)
return;
var id = match[1]; // id = skater id
var prefix = 'ScoreBoard.Team(' + t + ').Skater(' + id + ')'; //Example: prefix = ScoreBoard.Team('1').Skater('id')
if (k == prefix + '.Number') { //Example if skater id == ScoreBoard.Team('team').Skater('id').Number
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't hurt to keep

Copy link
Contributor Author

@AdamSmasherDerby AdamSmasherDerby Aug 18, 2018

Choose a reason for hiding this comment

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

It's wrong, though. I pulled all my old comments because they were largely incorrect. I'll put in a more coherent comment instead.

var fo_exp = ($($('.Team' + t + ' .Skater.Penalty[id=' + s + '] .BoxFO_EXP')[0]).data("id") != null);
// Change row colors for skaters on 5 or more penalties, or who have been expelled.
var cnt = 0;
var fo_exp = ($($('.Team' + t + ' .Skater.Penalty[id=' + s + '] .BoxFO_EXP')[0]).data("id") != null);
Copy link
Contributor

Choose a reason for hiding this comment

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

indentation is off here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not showing up this way in Eclipse, so I'm a little puzzled.

pd.append($('<div>').addClass('BoxFO_EXP').html('&nbsp;'));
pd.append($('<div>').addClass('Total').text('0'));
pd.append($('<div>').addClass('BoxFO_EXP').html('&nbsp;').css('color',teamFColor));
pd.append($('<div>').addClass('Total').text('0').css('color',teamFColor));
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this the best way to handle this?

The existing colour handling code is up around line 38, and we should try to keep with that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The existing color handling code at line 38 just assigns the colors to a custom class. Attempting to assign the custom class to the partial row where the penalty data lives breaks the appearance pretty badly. I'm open to other suggestions, but I've not been able to figure out another way to do this that doesn't look terrible

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's kind of overkill. The reason it's done that way in the SB is because it has to deal with the :before and :after pseudoclasses which can't be directly modified from the JS. If I could have used the approach I've proposed here for the SB, I totally would have.

Copy link
Contributor

Choose a reason for hiding this comment

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

Looking through the other dashboards, we appear to have a mix of techniques for this.

We should probably look at adding a utility function to manage the css classes for this, the XML side of the house has something vaguely in that area.

@brian-brazil brian-brazil merged commit 8ed320f into rollerderby:dev Aug 19, 2018
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.

2 participants