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

Fonts #11

Open
etpinard opened this issue Aug 30, 2017 · 4 comments
Open

Fonts #11

etpinard opened this issue Aug 30, 2017 · 4 comments

Comments

@etpinard
Copy link
Contributor

etpinard commented Aug 30, 2017

Unfortunately, we'll have to install them on the system, like we currently do here.

I thought that adding something like:

<style type="text/css">
    @import url('https://fonts.googleapis.com/css?family=Open+Sans:600,400,300,200|Droid+Sans|PT+Sans+Narrow|Gravitas+One|Droid+Sans+Mono|Droid+Serif|Raleway|Old+Standard+TT');
</style>

in the SVG's <defs> would work, but no. <img> with SVG src values don't import fonts, so Plotly.toImage fails to render the fonts correctly. See this article for more details.

cc #1

@etpinard
Copy link
Contributor Author

etpinard commented Sep 1, 2017

So, this here means that we'll (unfortunately) need docker for plotly.js image testing (see plotly/plotly.js#1972). Of course we could ask devs to install the fonts locally before running the tests, but that sounds even more painful than using docker.

I thought about removing all non-default-font-related mocks from our plotly.js image tests arguing that plotly.js is font-agnostic, so plotly.js testing should be font-agnostic. But, that will most likely lead to breakage down the road.

That said, our docker logic will be trimmed considerably. In theory, we should only need one docker run and one docker exec command to get things working. So we should say no more docker, but really a lot less docker.

@etpinard
Copy link
Contributor Author

etpinard commented Sep 1, 2017

Here's a side-by-side comparison of our font mocks:

https://github.com/plotly/plotly.js/compare/compare-fonts-baselines

In brief, the new baselines look a lot better than with the old image server 🎉

@etpinard
Copy link
Contributor Author

etpinard commented Oct 5, 2017

The image server side of this issue was resolved by @scjody 's #1 🎉

But, the plotly.js image testing side (cc plotly/plotly.js#1972) remains open. We'll need use a docker container in the plotly.js image test to ensure that (1) the correct fonts are used (2) the generate image are pixel-perfect in all operating system.

So, @scjody is there an easy way to reuse the deployment/Dockfile but without the last few lines? For plotly.js, we want to map the contents of the local plotly.js/ directory in the container and run command on-demand via docker exec. Perhaps we should create a image-test specific image?

@scjody
Copy link
Contributor

scjody commented Oct 5, 2017

is there an easy way to reuse the deployment/Dockfile but without the last few lines?

Yes - there are two main options:

  1. Since Docker has a concept of layers we can split the Dockerfile into two, with one containing the basics needed for both testing and production, and with the other adding to it the things needed for production. The only thing I don't like about this is that it means production builds become 2 steps, but since CI is running that side of things anyway it shouldn't be too bad.

  2. You could ignore the parts of the build you don't want (I don't understand why you wouldn't want the image exporter code and an npm install of its dependencies, but it's up to you). Docker allows overriding the command and entrypoint when you run the container and port exports don't happen by default.

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

No branches or pull requests

2 participants