Closed samreid closed 4 years ago
@brandonLi8 this would be a nice polish to avoid jitter in the ElapsedTimeNumberDisplay
We went with const numberFontFamily = 'font-family:Trebuchet MS,Lucida Grande,monospace;';
Feel free to promote it to a static attribute of StopwatchNode (or some other constant in scenery-phet) if you want to reuse it.
I went ahead and promoted numberFontFamily to a static attribute and used it in ElapseTimeNumberDisplay.
To my eye, it looks like there is still jitter but I'm not sure. Assigning @ariel-phet for review in master.
Also assigning back to @samreid for review of implementation.
No jitter on my browsers (Chrome and FF on Windows) and I can tell it is rendering Trebuchet MS. Looks good.
According to the documentation in Font.js, the multi-word font family names should be surrounded in quotes, like so:
var font = new scenery.Font( {
family: '"Times New Roman", serif',
style: 'italic',
lineHeight: 10
} );
And from the documentation:
// {string} - A comma-separated list of families, which can include generic families (preferably at the end) such
// as 'serif', 'sans-serif', 'cursive', 'fantasy' and 'monospace'. If there is any question about escaping (such
// as spaces in a font name), the family should be surrounded by double quotes.
According to https://stackoverflow.com/questions/13751412/why-would-font-names-need-quotes it is safe, and even recommended to include the quotes in the css font-family specification, so perhaps this can be just changed once in StopwatchNode.NUMBER_FONT_FAMILY
. When changing this, please test StopwatchNode using the scenery-phet demo harness to make sure it works properly.
Everything else looks great, back to you @brandonLi8
@samreid,
I tried
StopwatchNode.NUMBER_FONT_FAMILY = '"Trebuchet MS", "Lucida Grande", monospace';
but the scenery-phet demo wasn't working properly (ie it looked like
I then tried
StopwatchNode.NUMBER_FONT_FAMILY = 'Trebuchet MS, "Lucida Grande", monospace';
and the scenery-phet demo was working correctly.
So it looks like only Lucida Grande
allows quotes. @samreid How should I proceed? Either leave it as-is with no quotes on all of them or only quote Lucida Grande
?
@jonathanolson can you please recommend how to proceed for this issue?
Well, it looks like it was being provided to RichText, and the family was being inserted into a double-quoted string in the HTML:
"<span style="font-size: 20px; font-family:"Trebuchet MS", "Lucida Grande", monospace;">00:00</span><span style="font-size: 14px;font-family:"Trebuchet MS", "Lucida Grande", monospace;">.00</span>"
The proper way would be to use escapeHTML.js to encode those double-quotes to "
, however himalaya... doesn't like parsing that. So instead, it works in this case to single-quote the style attribute, so we can safely use double-quotes inside:
"<span style='font-size: 20px; font-family:"Trebuchet MS", "Lucida Grande", monospace;'>00:00</span><span style='font-size: 14px;font-family:"Trebuchet MS", "Lucida Grande", monospace;'>.00</span>"
Committed above. @samreid can you briefly check/review?
I inspected the commits, and tested in the scenery-phet harness (where I identified the new issue linked above), and in Gravity and Orbits. Looks great, thanks! Closing.
From https://github.com/phetsims/scenery-phet/issues/616, @ariel-phet pointed out that /collision-lab/js/common/view/ElapsedTimeNumberDisplay.js sometimes shows a jitter. We are considering other fonts for that issue and, once decided, it seems like Collision Lab should use the same font. Perhaps we will factor it out to a constant like "NUMBER_FONT".