Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
#6990Add support for numeric font
weight
#6990Changes from all commits
37dee48
3212866
448eb80
3a3f458
15b25a5
be24a14
d8424a3
353efc4
abb3fc8
8266a6d
46e6b27
3e4942a
72044b5
4d52885
b92ef23
b125396
54005b9
09f4dd3
f67b40c
99162e5
990fa8d
f3c0356
a5cc7f8
63824c1
190aef1
091e7d3
82de3ff
59779f3
f6fcbd7
10f477f
82863fd
bff00ac
48d057f
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 forflaglist
)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.
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 ofweight
noting that forarrayOk
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.
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 toRegular
.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
orbold
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.
@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 tomaplibre-gl-js
which is the continuation ofmapb0x-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 tomaplibre
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:
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 :)
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 toKlokantech Noto Sans
if it's notMetropolis
orOpen 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.
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 stringMetropolis
is not in the supported fonts list, and so it will get replaced withOpen 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
andstyle
options, would it make sense to change that behavior? Since a user could passfamily: 'Metropolis', weight: 'bold'
and we could use that to determine they want the font faceMetropolis 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.