-
Notifications
You must be signed in to change notification settings - Fork 1
feat: publish charts #36
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Please disregard my previous comments on defining default Less variables. We already search for all existing variables and assign an initial value to them (which I just updated to For cases where we need a fallback value we can define it like this, if .chart {
background: blue;
background: @vis_d3-lines_bg;
} |
bffbb52
to
1c5a07a
Compare
40c090b
to
31f53fd
Compare
c21cb7a
to
f201652
Compare
d59f668
to
c88f5d5
Compare
Fix css postprocess
* add chart public version as suffix to publish action log * move authorization back to publish route
|
||
server.method('getModel', name => ORM.db.models[name]); |
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 think at some point it would be nice to start to document all these server methods in the API readme so developers know they exist and what exactly they do.
delete publishData.chart; | ||
|
||
if (!data) { | ||
await log('error-data'); |
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.
These strings are displayed to the user during publishing. So it's essential that all the keys that are logged here are translated in our translation spreadsheet with a publish / status /
prefix. e.g. for this log there needs to be an entry publish / status / error-data
that explains what happened.
*/ | ||
const vis = visualizations.get(chart.type); | ||
if (!vis) { | ||
await log('error-vis-not-supported'); |
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.
same here, the translation for publish / status / error-vis-not-supported
is missing.
…ins (#155) * api handles chart access authorization only, the rest is done by plugins * bump orm to 3.14.2 and chart-core to 8.8.1 * load plugins before routes (so they can influence the Joi schema) * only allow formats that are registered by plugins * find first results with data
* fix: remove woff2 for now from font-face generation * fix test
this will allows us to "wait" for things like screenshots. by passing the log function plugins are also allowed to communicate additional steps in the publishing process, eg "you chart is being exported as PNG"
Very similar to datawrapper/frontend#1. Both PRs are concerned with rendering charts, the frontend dynamically and the api renders to files, that are saved locally or on s3. Depending on which plugin is used for file storage.
To actually run this code, changes in visualization plugins is required.
https://github.com/datawrapper/plugin-d3-lines/pull/11
https://github.com/datawrapper/plugin-d3-scatter-plot/tree/node-publishing
https://github.com/datawrapper/plugin-d3-pies/tree/no-extend
https://github.com/datawrapper/plugin-d3-bars/tree/no-extend
https://github.com/datawrapper/plugin-tables/tree/node-publishing
https://github.com/datawrapper/plugin-visualization-column-charts/tree/node-publishing
https://github.com/datawrapper/plugin-visualization-pie-chart
https://github.com/datawrapper/plugin-locator-maps/tree/node-publishing
https://github.com/datawrapper/plugin-d3-maps/tree/node-publishing
More will follow, thanks to @ilokhov! 🎉
Some design decisions I have made:
pugjs
for baseindex.html
Pug is a node templating engine I have used in the past (when it was still called Jade). It seemed like a nice fit and works well here. When I started with this task there was a lot more code in
index.pug
, this is very reduced now and it might be worth to check out if we can get rid of pug and use only classic JS template strings.Calling the PHP API from Node
I needed a way to get basic visualization info (location of static files and render code) from plugins. @davidkokkelink (thank you!) had the idea to use the existing endpoint of the PHP API as long as it exists. At some point we might need to rebuild this is Node but it gave me a nice productivity boost.
Exposing style compilation as endpoint
I added
GET /visualizations/:id/styles.css
. It is used by the frontend to dynamically request CSS for a visualization based on type and theme. Since the code was already implemented for the API and it relies on Node APIs, it made sense to expose that functionality. There is a little bit of duplicated code in the publish function and this endpoints handler method. This saves a HTTP request, there is an opportunity to refactor but it's not a lot of code.Always writing to disk first
The publish method will always write the published folder into a temp directory (
os.tmpdir()
) and then move that folder somewhere else locally or to S3. This decision reduced the complexity in the corresponding pluginschart-data-local
andchart-data-s3
. It could directly copy/upload into the destination directory but that makes the code less isomorphic. It seemed like a good tradeoff for me and is an opportunity for optimization later on.Custom Less variable search
The Less compiler needs every occurrence of a variable to be defined somewhere in the stylesheet, otherwise it will throw an error and not compile. This was solved in PHP by using a Twig preprocess step that checked for variable definitions and removed the Less rules when the variable was undefined. In Node we didn't want to use Twig. In the beginning I removed all Twig Syntax and defined variables with
initial
at the top of each.less
file. This get's cumbersome and error prone when introducing new variables.The current approach does this automatically by searching variables in the Less AST and adding initial values before compiling. After the compilation a PostCSS step removes all declarations that have the initial value that we don't need. The PostCSS step also takes care of prefixing vendor rules, minifying CSS and setting a default unit for unit-less values (
12 -> 12px
).