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

Add "bold" weight, "italic" style and "small-caps" variant to fonts #6956

Merged
merged 102 commits into from
Apr 23, 2024

Conversation

archmoj
Copy link
Contributor

@archmoj archmoj commented Apr 11, 2024

Resolves #4646.

Support various font styling options all over the place 🎉 Enjoy!

@plotly/plotly_js
cc: @ndrezn

TODO:

  • implement in scattergl
  • implement in scatter3d
  • implement in gl3d axes
  • implement in gl2d axes i.e. heatmapgl
  • support weight and style in mapbox
  • limit variant options in gl3d, gl2d and regl traces

No longer needed:

@archmoj
Copy link
Contributor Author

archmoj commented Apr 22, 2024

This PR is ready for review 🔍

@archmoj archmoj changed the title Add weight, style and variant to fonts Add *bold* weight, *italic* style and small-caps* variant` to fonts Apr 22, 2024
@archmoj archmoj changed the title Add *bold* weight, *italic* style and small-caps* variant` to fonts Add "bold" weight, "italic" style and "small-caps" variant to fonts Apr 22, 2024
editType: 'calc',
colorEditType: 'style',
arrayOk: true,
variantValues: ['normal', 'small-caps'],
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason for limiting the variants here?

Or, should we maybe just only support these two variants everywhere?

Not sure just wondering.

Copy link
Contributor

Choose a reason for hiding this comment

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

Aha, this is because of WebGL I'm guessing?

Maybe make it a global constant value like webGLSupportedVariants?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

WebGL traces different mechanism to render text i.e. via CanvasRenderingContext2D https://developer.mozilla.org/en-US/docs/Web/API/CanvasRenderingContext2D/font

At the moment only some of those options are supported.
So we should have it this way so that for those specific traces and subplots (gl2d, gl3d and mapbox) the variant options are limited.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Aha, this is because of WebGL I'm guessing?

Maybe make it a global constant value like webGLSupportedVariants?

Yes because of WebGL pipelines.
But no to have a global variable since scattergl, gl2d, gl3d and mapbox each uses different pipeline.
At one point we may be able to add one option to one not the other.
So I think it's better to keep it this way.

@@ -13243,10 +13308,18 @@ proto.update = function(bounds, labels, labelFont, ticks, tickFont) {
if(!ticks[d][i].text) {
continue
}

var font = {
family: ticks[d][i].font || tickFont[d].family,
Copy link
Contributor

Choose a reason for hiding this comment

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

why does this line say ticks[d][i] instead of tickFont[d] like the line below?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change is reflected from https://github.com/gl-vis/gl-axes3d/pull/24/files
gl-axes3d has its own API and there was a bug there which didn't pick the right item.
This is fixed now in that PR.

"gl-cone3d": "^1.6.0",
"gl-error3d": "^1.0.16",
"gl-heatmap2d": "^1.1.1",
"gl-line3d": "1.2.1",
"gl-mesh3d": "^2.3.1",
"gl-plot2d": "^1.4.5",
"gl-plot2d": "github:gl-vis/gl-plot2d#ac02a12ec9fa5283a79c042b900dfd0707c43891",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@@ -13,15 +13,16 @@
"box-intersect": "plotly/box-intersect#v1.1.0",
"convex-hull": "^1.0.3",
"delaunay-triangulate": "^1.1.6",
"gl-axes3d": "github:gl-vis/gl-axes3d#dae56c2e455a3f754f7dfe9aa74389a109df08a9",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

"gl-plot3d": "^2.4.7",
"gl-pointcloud2d": "^1.0.3",
"gl-scatter3d": "^1.2.3",
"gl-scatter3d": "github:gl-vis/gl-scatter3d#9504c1bff7d9cfed8a85866cbc5e47cb32bdfb9a",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Unicase looks terrible 🙈 Is that how it's supposed to look? The rest look fine haha.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes it should look just like that.
I kind of like it ;)

@emilykl
Copy link
Contributor

emilykl commented Apr 23, 2024

💪 💪 💪 This is really awesome @archmoj . 💯 Huge improvement for Plotly.js. No more blocking comments from me.

IMO it would be ideal to have a few more tests which set a font style/weight/variant for just part of the plot rather than just setting the global layout font, e.g. by using tickfont, titlefont, etc. But I defer to your judgment on that.

Copy link
Contributor

@emilykl emilykl left a comment

Choose a reason for hiding this comment

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

🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

text weight
5 participants