-
Notifications
You must be signed in to change notification settings - Fork 472
⚡️Enhance/html copy to download #1669
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
Conversation
98ffee7 to
aac1cca
Compare
|
Size Change: +26.8 kB (+0.65%) Total Size: 4.14 MB
|
c1b12d1 to
999c814
Compare
AntoLC
left a comment
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 have some cases where the mp3 and videos are not exported, I used this doc model:
https://www.blocknotejs.org/docs/features/blocks
src/frontend/apps/impress/src/features/docs/doc-header/__tests__/DocToolBox.spec.tsx
Outdated
Show resolved
Hide resolved
src/frontend/apps/impress/src/features/docs/doc-header/__tests__/DocToolBoxLicence.spec.tsx
Outdated
Show resolved
Hide resolved
src/frontend/apps/impress/src/features/docs/doc-export/__tests__/ExportMIT.test.tsx
Outdated
Show resolved
Hide resolved
src/frontend/apps/impress/src/features/docs/doc-export/components/ModalExport.tsx
Outdated
Show resolved
Hide resolved
| const fetched = await exportCorsResolveFileUrl(doc.id, src); | ||
|
|
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 can see you use the CORS proxy, but I think it works only with images:
docs/src/backend/core/api/viewsets.py
Lines 1551 to 1555 in 999c814
| if not content_type.startswith("image/"): | |
| return drf.response.Response( | |
| status=status.HTTP_415_UNSUPPORTED_MEDIA_TYPE | |
| ) | |
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.
Oh yes, I see, the current CORS proxy only supports images.
So that means external audio / video (when they’re not uploaded to our backend) are rejected and can’t be added to the HTML ZIP? ( as you said sometimes mp3 and vidéos are not exported )
In that case, should we update the backend proxy to also allow audio and video so these files can be exported too?
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.
There are 2 types of "link" (internal / external), if the resources are internal like https://docs.numerique.gouv.fr/media/..., we have to download them and put them in the zip files because they will not be accessible otherwise; on the other hand, if the resources are external like https://youtube.com/kefseklsfes.mp4, I don't think we should download them but just let the src as it was, moreover we don't have the hand on the external resources they could be huge (1go) or even infected.
src/frontend/apps/impress/src/features/docs/doc-export/components/ModalExport.tsx
Outdated
Show resolved
Hide resolved
src/frontend/apps/impress/src/features/docs/doc-export/components/ModalExport.tsx
Outdated
Show resolved
Hide resolved
src/frontend/apps/impress/src/features/docs/doc-export/components/ModalExport.tsx
Outdated
Show resolved
Hide resolved
| const zipBuffer = await cs.toBuffer(await download.createReadStream()); | ||
|
|
||
| // ZIP files start with "PK\x03\x04" | ||
| expect(zipBuffer.length).toBeGreaterThan(4); | ||
| expect(zipBuffer[0]).toBe(0x50); | ||
| expect(zipBuffer[1]).toBe(0x4b); |
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.
Could we improve this part to check what is inside the zip file ?
The idea:
import JSZip from 'jszip';
// In your test:
const zipBuffer = await cs.toBuffer(await download.createReadStream());
// Unzip and inspect contents
const zip = await JSZip.loadAsync(zipBuffer);
// Check that index.html exists
const indexHtml = zip.file('index.html');
expect(indexHtml).not.toBeNull();
// Read and verify HTML content
const htmlContent = await indexHtml!.async('string');
expect(htmlContent).toContain('Hello HTML ZIP');
// Check for media files
const mediaFiles = zip.file(/^media\//);
expect(mediaFiles.length).toBeGreaterThan(0);
// Verify the SVG image is included
const svgFile = mediaFiles.find(f => f.name.endsWith('.svg'));
expect(svgFile).toBeDefined();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.
yes I agree this is much better
fbdb37c to
882b7f3
Compare
882b7f3 to
22ff23f
Compare
src/frontend/apps/impress/src/features/docs/doc-export/__tests__/ExportMIT.test.tsx
Outdated
Show resolved
Hide resolved
| const fetched = await exportCorsResolveFileUrl(doc.id, src); | ||
|
|
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.
There are 2 types of "link" (internal / external), if the resources are internal like https://docs.numerique.gouv.fr/media/..., we have to download them and put them in the zip files because they will not be accessible otherwise; on the other hand, if the resources are external like https://youtube.com/kefseklsfes.mp4, I don't think we should download them but just let the src as it was, moreover we don't have the hand on the external resources they could be huge (1go) or even infected.
22ff23f to
82629f7
Compare
AntoLC
left a comment
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.
We should open a issue to add a css in the zip file to get the look and feel of what we have in Docs; because it seems correct on a accessibility perspective (dom) but visually it is not usable.
014fec0 to
f0f499a
Compare
makes the option less visible as it's not useful to most users Signed-off-by: Cyril <[email protected]>
replaced “copy as html” with export modal option and full media zip export Signed-off-by: Cyril <[email protected]>
checks generated zip contains html and embedded media files Signed-off-by: Cyril <[email protected]>
f0f499a to
9b03754
Compare
Purpose
Enable users to export an accessible HTML version of content, replacing the "Copy as HTML" option with a full export feature from the modal.
issue : 599
Proposal