-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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 support for numeric font weight
#6990
Conversation
@@ -114,8 +114,13 @@ exports.valObjectMeta = { | |||
'are coerced to the `dflt`.' | |||
].join(' '), | |||
requiredOpts: [], | |||
otherOpts: ['dflt', 'min', 'max', 'arrayOk'], | |||
otherOpts: ['dflt', 'min', 'max', 'arrayOk', 'extras'], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@archmoj Is this adding a new functionality to coerce
, where numeric (or other) properties can also be configured to accept a list of 'extra' values?
That's pretty cool -- is there anywhere we should document that internally?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question.
Yes that's true.
We have other types that support the extras
option so I think it's clear internally.
But on plotly.py we need to see if this changes would be reflected properly by codegen.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point.
Makes me think, it would be awesome if we could build checking the Plotly.py codegen into the CI process. It would have to produce some kind of output that's easy to verify. It could run only if the schema.json
has changed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps add a line in the docstring here explaining extras
for integers (like the explanation here for flaglist
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For flag lists combination mean something. That's why it is commented.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For this mock, maybe use a font that has more available weights, to test that they work properly in WebGL.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I was interested to test it with the default font as that's what most users use at this moment.
…nd gl-text" This reverts commit abb3fc8.
OK. This is ready for review. |
|
||
// Must use one of the following fonts as the family, else default to 'Open Sans Regular' | ||
// See https://github.com/openmaptiles/fonts/blob/gh-pages/fontstacks.json | ||
var supportedFonts = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@archmoj Does Mapbox make their list of supported fonts public anywhere as part of their package? Just wondering if there's a way we can ensure this list is always up-to-date without updating it manually.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No in fact we use latest v1 version of mapbox-gl
. Due to the licence changes of their v2+ no significant v1 is expected. On the other hand we may switch to maplibre-gl-js
which is the continuation of mapb0x-gl
v1. See #5578.
BTW we could possibly improve our docs for these fonts in a separate PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK got it. Do you know to what extent maplibre
supports different font weights? Basically, will adding these options make switching to maplibre
significantly more complicated?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think so. It's a kind of problem we could address when/if we switch to it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@emilykl , there is a migration branch in here maplibre where we should be able to try out:
else if(weight > 350) str += ' Regular'; | ||
else str += ' Light'; | ||
} else { // Other families | ||
str = 'Klokantech Noto Sans'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't this else
mean the font will default to Klokantech Noto Sans
if it's not Metropolis
or Open Sans
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh I see, maybe not, because it has already passed through the supplyDefaults
step -- can you confirm @archmoj ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. No that shouldn't happen because further down we test and ensure the font is supported.
I could restrict this else
to make it read better?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed in bff00ac.
handleTextDefaults(traceIn, traceOut, layout, coerce, | ||
{ | ||
noSelect: true, | ||
noFontVariant: true, | ||
font: { | ||
family: supportedFonts.indexOf(layout.font.family) !== -1 ? layout.font.family : 'Open Sans Regular', | ||
family: isSupportedFont(layoutFontFamily) ? layoutFontFamily : 'Open Sans Regular', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, naive question because I don't quite understand -- if the user passes just Metropolis
(for example) as the font family, won't that register as "not supported" because the string Metropolis
is not in the supported fonts list, and so it will get replaced with Open Sans
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that we have weight
and style
options, would it make sense to change that behavior? Since a user could pass family: 'Metropolis', weight: 'bold'
and we could use that to determine they want the font face Metropolis Bold
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I was thinking about that and I'd be happy to work on it in a separate PR.
Obviously we don't want to break previous mocks.
Instead we could work on "adding support for minimal family names in mapbox
".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, that's fine to do in another PR. Would be nice to have!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tracked in #6993.
@@ -216,7 +216,7 @@ function convertTextStyle(gd, trace) { | |||
optsOut.font = { | |||
size: tfs * plotGlPixelRatio, | |||
family: tff, | |||
weight: tfw, | |||
weight: weightFallBack(tfw), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this needed if we are already disallowing numeric weight values at the schema level?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question.
This is for scattergl
with limited support of weight
noting that for arrayOk
attributes we do not check every element at the supply defaults step which could result in performance issues during interactions with the plot.
As a result this fall back is needed to map unsupported values in arrays to supported options.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
gotcha, thank you. Could you perhaps add that as a comment above the function definition?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed in 48d057f.
else if(weight > 250) str += ' Light'; | ||
else if(weight > 150) str += ' Extra Light'; | ||
else str += ' Thin'; | ||
} else if(parts.slice(0, 2).join(' ') === 'Open Sans') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Couldn't this be family.startsWith('Open Sans')
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. That may break IE11 which is fine as of today :)
@@ -225,6 +225,12 @@ function convertTextStyle(gd, trace) { | |||
return optsOut; | |||
} | |||
|
|||
function weightFallBack(w) { | |||
if(w <= 1000) { | |||
return w > 500 ? 'bold' : 'normal'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't the threshold be 400?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
500 is Medium
looks very similar to Regular
.
IMHO it's debatable between 450/500 to 550.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, ok, I guess if the only options are normal
or bold
we have to draw the line somewhere. Maybe just add a comment explaining why 500 is chosen as the threshold.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it not possible to use numeric weights in gl2d
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. For heatmapgl
gl2d scene the numeric values work nicely similar to gl3d
scenes and scatter3d
.
Also for scattergl
(since it uses a different pipeline) it works using the fallback which maps the values to bold
and normal
i.e. limited support.
@archmoj This is looking really good. 🎉 Could you summarize which trace types have limited/no support for numeric font weights? This still needs to be tested with the Plotly.py codegen, right? |
Good call. |
Good call. Added to the PR description: |
This PR adds numeric
weight
options to various fonts.Please note that
scattergl
traces has limited support of numeric values i.e. the values are mapped to eitherbold
ornormal
.TODO:
Extrabold
instead ofExtra Bold
? openmaptiles/fonts#24 considering current bug with extra bold usingOpen Sans
withmapbox
mapbox
scattergl
i.e. 400 --> 'normal', 700 --> 'bold'Also opened Missing valid integers between 1 and 1000 jednano/css-font-weight-keywords#1
gl3d
base64
font.weight
in gl3d@plotly/plotly_js
cc: @ndrezn