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

Fix some PDF related issues #133

Merged
merged 4 commits into from
Aug 9, 2024
Merged

Fix some PDF related issues #133

merged 4 commits into from
Aug 9, 2024

Conversation

Sinclair19
Copy link
Contributor

@Sinclair19 Sinclair19 commented Aug 8, 2024

  1. f0d4650 Fixed the ./build-meta.bash: 7: [[: not found error occurring in both pnpm dev and build processes.

  2. 623e35c Resolved an issue where navigating to another view (PDF, both, log) and clicking on a path did not automatically redirect to the editor view, which was unintended behavior.

  3. fa7ade7 Fixed issues with PDF display and the functionality of the both view. Most modern desktop browsers support using iframes to display PDF files, which is not supported by mobile browsers. However this is still a big improvement over the previous not usable state (Before the pdf view will freeze the whole page except the nav bar and not work on both view). The pdfslick dependency was retained and can be removed in the future.

Currently pdfslick has problem with interface.
ifame pdf works well but it depends on browser and can't work on mobile
For now just use iframe way
fix `./build-meta.bash: 7: [[: not found`
fix build-meta.ts generate syntax problem
Currently, when you navigate to another view and click on a path, it doesn't automatically redirect to the editor view, which is not the intended behavior
@Sinclair19 Sinclair19 changed the title Fixed some PDF related issues Fix some PDF related issues Aug 8, 2024
Copy link
Collaborator

@zjkmxy zjkmxy left a comment

Choose a reason for hiding this comment

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

LGTM. Let me test before merge.

@zjkmxy zjkmxy merged commit 639e4dd into UCLA-IRL:main Aug 9, 2024
1 check passed
@zjkmxy
Copy link
Collaborator

zjkmxy commented Aug 9, 2024

which is not supported by mobile browsers.

I hope this does not break things. I'm not able to test it on my phone or iPad before the code is released.

@Sinclair19
Copy link
Contributor Author

Sinclair19 commented Aug 9, 2024

which is not supported by mobile browsers.

I hope this does not break things. I'm not able to test it on my phone or iPad before the code is released.

I think it won't be a big problem.
You can have a try of this demo website. It provides different ways to embed pdf, which includes iframe.
https://pdfobject.com/static/
On Android Chrome/Edge, it just shows a button to open/download it.
On Android Firefox, it can be displayed properly.

@zjkmxy
Copy link
Collaborator

zjkmxy commented Aug 9, 2024

which is not supported by mobile browsers.

I hope this does not break things. I'm not able to test it on my phone or iPad before the code is released.

I think it won't be a big problem. You can have a try of this demo website. It provides different ways to embed pdf, which includes iframe. https://pdfobject.com/static/ On Android Chrome/Edge, it just shows a button to open/download it. On Android Firefox, it can be displayed properly.

It also works fine on iPad as an installed PWA. I think it's good.

@Sinclair19
Copy link
Contributor Author

Great, I think after it being released and tested, we can remove all PDFSlick related dependency. I can make another PR to do this.

@zjkmxy
Copy link
Collaborator

zjkmxy commented Aug 9, 2024

Great, I think after it being released and tested, we can remove all PDFSlick related dependency. I can make another PR to do this.

I have already done. But thanks.

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

Successfully merging this pull request may close these issues.

2 participants