Skip to content

fix PDF rendering issue #70

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

fix PDF rendering issue #70

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 21 additions & 16 deletions src/report_table.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ const loadStylesheet = function (link) {
document.getElementsByTagName('head')[0].appendChild(linkElement);
};

const buildReportTable = function (
const buildReportTable = async function (
config,
dataTable,
updateColumnOrder,
Expand All @@ -42,7 +42,7 @@ const buildReportTable = function (
const chartCentreX = bounds.x + bounds.width / 2;
const chartCentreY = bounds.y + bounds.height / 2;

removeStyles().then(() => {
await removeStyles().then(() => {
Copy link

Choose a reason for hiding this comment

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

medium

Using await with .then() is an anti-pattern1 as it mixes two different asynchronous handling styles. Since buildReportTable is an async function, it would be more idiomatic and readable to use await directly.

  await removeStyles();

Style Guide References

Footnotes

  1. Avoid mixing await with .then() for asynchronous operations. (link) ↩

Copy link
Author

Choose a reason for hiding this comment

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

Agreed but I did not want to refactor that much code. I'm happy to let the content team do more work on this

if (
typeof config.customTheme !== 'undefined' &&
config.customTheme &&
Expand Down Expand Up @@ -228,17 +228,17 @@ const buildReportTable = function (
.style('font-size', config.headerFontSize + 'px')
.attr('draggable', true)
.call(drag)
.on('mouseover', function(cell) {
.on('mouseover', function (cell) {
d3.select('#tooltip')
.style('left', d3.event.x + 'px')
.style('top', d3.event.y + 'px')
.html(cell.label);
.style('left', d3.event.x + 'px')
.style('top', d3.event.y + 'px')
.html(cell.label);
d3.select('#tooltip').classed('hidden', false);
return dropTarget = cell
return (dropTarget = cell);
})
.on('mouseout', function(cell) {
.on('mouseout', function (cell) {
d3.select('#tooltip').classed('hidden', true);
return dropTarget = null
return (dropTarget = null);
});

var table_rows = table
Expand Down Expand Up @@ -527,7 +527,7 @@ const buildReportTable = function (
);
};

renderTable().then(() => {
await renderTable().then(() => {
Copy link

Choose a reason for hiding this comment

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

high

The addOverlay() call on line 534 within this then block is not awaited. Since addOverlay performs animations, not awaiting it means buildReportTable will resolve and done() will be called before the animations are complete. This likely means the PDF rendering issue will persist. To fix this, you should await addOverlay().

  await renderTable();

Copy link
Author

Choose a reason for hiding this comment

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

The animate custom theme should NOT be used if the visualization is rendered. Thats a larger change

document.getElementById('reportTable').classList.add('reveal');
if (config.customTheme === 'animate') {
document.getElementById('visSvg').classList.remove('hidden');
Expand Down Expand Up @@ -639,12 +639,17 @@ looker.plugins.visualizations.add({
// console.log(config)
var dataTable = new VisPluginTableModel(data, queryResponse, config);
this.trigger('registerOptions', dataTable.getConfigOptions());
buildReportTable(config, dataTable, updateColumnOrder, element);

// DEBUG OUTPUT AND DONE
// console.log('dataTable', dataTable)
// console.log('container', document.getElementById('visContainer').parentNode)
buildReportTable(config, dataTable, updateColumnOrder, element)
.then(() => {
// DEBUG OUTPUT AND DONE
Copy link

Choose a reason for hiding this comment

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

Are you intending to keep these comments in the code?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, it appears to be to aid debubugging

// console.log('dataTable', dataTable)
// console.log('container', document.getElementById('visContainer').parentNode)

done();
done();
})
.catch(error => {
console.error(error);
done();
});
Comment on lines +642 to +653
Copy link

Choose a reason for hiding this comment

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

medium

To avoid repeating the done() call in both the success and error paths, you could use .finally()1. This would make the code slightly cleaner and more maintainable by ensuring done() is always called, regardless of the outcome.

buildReportTable(config, dataTable, updateColumnOrder, element)
  .then(() => {
    // DEBUG OUTPUT AND DONE
    // console.log('dataTable', dataTable)
    // console.log('container', document.getElementById('visContainer').parentNode)

  })
  .catch(error => {
    console.error(error);
  })
  .finally(() => {
    done();
  });

Style Guide References

Footnotes

  1. Use .finally() to avoid repeating code in both the success and error paths. (link) ↩

Copy link

Choose a reason for hiding this comment

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

That's a pretty good suggestion

Copy link
Author

Choose a reason for hiding this comment

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

I thought about it but I could not be arsed to do it

},
});