NASA-IMPACT / nasa-apt-frontend

Web application for NASA-APT
Apache License 2.0
4 stars 2 forks source link

Fix equations and images captions in document PDF preview #558

Closed vgeorge closed 1 year ago

vgeorge commented 1 year ago

This fixes a regression introduced in https://github.com/NASA-IMPACT/nasa-apt-frontend/pull/549 (reported here) where the captions and number for images and equations disappeared in the document PDF preview.

Please note this PR only reinstates the preexisting number in document PDF preview. The following issues are still present:

I suggest we handle those separately as they look more involved.

To test, open a document in document preview mode and confirm numbers are present for images, equations, and tables. Example:

cc @wrynearson @sunu @kamicut

vgeorge commented 1 year ago

I added more changes to this PR. This is now fixing numbering and caption positioning on both PDF exports in preview mode. To check, visit:

http://localhost:9000/documents/links/v1.0/pdf-preview http://localhost:9000/documents/links/v1.0/journal-pdf-preview

While these changes work in preview mode, the PDF file exported by the API do not have numbering. I'm not sure if this will be fixed once we merge this PR. @sunu would you know about this?

An additional note on this issue, we are using an approach of editing the DOM outside React using query selectors here which makes this page very hard debug and maintain. I left as is for now to avoid a big refactor, but in my opinion we should avoid this approach and refactor when possible.

sunu commented 1 year ago

While these changes work in preview mode, the PDF file exported by the API do not have numbering. I'm not sure if this will be fixed once we merge this PR. @sunu would you know about this?

@vgeorge that's fine. When the local setup is running against the staging backend, the staging backend uses the staging frontend to generate the PDFs. That's why the generated PDF doesn't reflect the local changes. But it will work as expected when the frontend changes are deployed to staging after we merge the PR.

An additional note on this issue, we are using an approach of editing the DOM outside React using query selectors here which makes this page very hard debug and maintain. I left as is for now to avoid a big refactor, but in my opinion we should avoid this approach and refactor when possible.

I totally agree and I think that's what we do by using the NumberingContext stuff for rendering journal PDF. eg: https://github.com/NASA-IMPACT/nasa-apt-frontend/blob/57ca4dddc4edc3a5fbad68010bb5c52da27756b6/app/assets/scripts/components/documents/journal-pdf-preview/index.js#L315. It manages the numbering inside react instead of outside of it. And I think we should use the same approach for document PDF and view mode to avoid regressions like this in the future.

cc @frozenhelium - this is the issue I was talking about on the call today.

wrynearson commented 1 year ago

@vgeorge @sunu is this ready for review/merging? LMK if you need any action from me. Feel free to merge without my review.

vgeorge commented 1 year ago

@wrynearson @kamicut @sunu I've added you to the document I linked in this post. If you see the correct numbering this is ready to merge in my opinion. We can look into the numbering context in separate as an enhancement.

wrynearson commented 1 year ago

Thanks @vgeorge. I won't get to this today. I'll review it on Monday.

vgeorge commented 1 year ago

I'll go ahead and merge to have the current enhancements on staging. I confirm the equation number is not working well in journal PDF and will look in separate.