Remove legacy methods for rendering the PDF documents#96
Conversation
|
The code itself seems fine, but I'm just concerned about other users since this would be a breaking change. |
|
It's not for now, more in May, and yes, it will break things, so I'm thinking of releasing this as version 0.1.0. |
f06b427 to
d8cb18c
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR removes all legacy iframe/embed rendering methods and related tests, constants, and documentation to simplify the component to only the “unwrap” rendering option.
- Deleted all E2E tests and example apps for legacy
iframe/embedrendering - Removed legacy rendering branches in
PdfViewer.vueand parameters in__init__.py - Updated README and CI workflow to drop legacy references and adjust the build matrix
Reviewed Changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_iframe_.py & tests/test_embed_.py | Removed legacy rendering tests |
| tests/streamlit_apps/example_*.py | Deleted legacy example apps |
| streamlit_pdf_viewer/frontend/src/PdfViewer.vue | Stripped out legacy_iframe & legacy_embed branches |
| streamlit_pdf_viewer/init.py | Removed rendering constants & rendering parameter |
| README.md | Cleaned up docs to drop legacy mentions |
| .github/workflows/ci-build.yml | Expanded OS matrix and pruned Python versions |
Comments suppressed due to low confidence (2)
streamlit_pdf_viewer/frontend/src/PdfViewer.vue:2
- The new default unwrap rendering is not covered by any existing tests. Consider adding a unit or E2E test to verify that the PDF renders correctly when using the default
unwrapmethod.
<div id="pdfContainer" :style="pdfContainerStyle">
streamlit_pdf_viewer/frontend/src/PdfViewer.vue:3
- Ensure the closing
</div>tags match the updated opening tags; removing the conditional wrappers may have left mismatched or orphaned tags, which could break the component's template.
<div id="pdfViewer" :style="pdfViewerStyle"></div>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| needs: [ build ] | ||
| runs-on: ubuntu-latest | ||
|
|
||
| steps: | ||
| - name: Coveralls | ||
| uses: coverallsapp/github-action@v2 | ||
| with: | ||
| parallel-finished: true |
Check warning
Code scanning / CodeQL
Workflow does not contain permissions Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 1 year ago
To fix the issue, we need to add a permissions block to the workflow file. This block should specify the minimal permissions required for the workflow to operate. Based on the workflow's operations, it appears that only contents: read is necessary for most jobs, as the workflow primarily reads files and runs tests. For jobs interacting with external services like Coveralls, additional permissions may be required, but these should be limited to the specific needs of the job.
The permissions block can be added at the root level of the workflow to apply to all jobs or at the job level for more granular control. In this case, we will add it at the root level for simplicity, while ensuring it covers all jobs appropriately.
| @@ -8,2 +8,4 @@ | ||
|
|
||
| permissions: | ||
| contents: read | ||
|
|
# Conflicts: # README.md # streamlit_pdf_viewer/__init__.py # streamlit_pdf_viewer/frontend/src/PdfViewer.vue
| <div v-else> | ||
| Error rendering option. | ||
| </div> | ||
| <div v-if="args.rendering === 'unwrap'" class="zoom-controls"> |
There was a problem hiding this comment.
@lfoppiano Reviewing this to merge with PR #110 I noticed that the class zoom-controls is not used anymore in the new version, even thought the CSS code still defines it and there is JS logic based on it (handleClickOutside).
Was this removal intentional or a mistake? It feels like something may be incomplete. I'd like to know to decide whether to keep it removed or add it back in my PR
There was a problem hiding this comment.
Probably a mistake 😓 this is not released, though, I did not finish fixing the tests and ran out of time...
There was a problem hiding this comment.
That's what I guessed, that it hadn't been fully tested. When I tested the other PR it really looked like this class is needed in a parent div surrounding everything, otherwise the zoom panel does not show up when the zoom button is clicked.
This PR removes the legacy methods for rendering the PDF documents.