-
Notifications
You must be signed in to change notification settings - Fork 471
⚡️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
base: main
Are you sure you want to change the base?
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
|
|
||
| expect(Export.default).toBeUndefined(); | ||
| }, 10000); | ||
| }, 60000); |
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.
Same, is 1mn really necessary, seems big ?
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.
The test was a bit flaky, I saw it fail occasionally and one run even took ~9s, so I put timeout to 1min a bit aggressively 😅
We could probably reduce it to 15–20s now ? Wdyt
| }); | ||
|
|
||
| test('It checks the copy as HTML button', async ({ page, browserName }) => { | ||
| test('It no longer shows the copy as HTML button', async ({ |
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.
You can remove totally this test, we don't assert things that does not exist anymore.
|
|
||
| blobExport = await exporter.toODTDocument(exportDocument); | ||
| } else if (format === DocDownloadFormat.HTML) { | ||
| const editorHtml = await editor.blocksToHTMLLossy(); |
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.
| const editorHtml = await editor.blocksToHTMLLossy(); | |
| const editorHtml = editor.blocksToHTMLLossy(); |
Have you seen that you have editor.blocksToFullHTML available as well ?
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, indeed, I’ve just switched to editor.blocksToFullHTML(), and it preserves the original document structure much better than blocksToHTMLLossy.
This is the best option BlockNote provides, I think.
| 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?
| // Ensure the filename has an extension consistent with the blob MIME type. | ||
| const mimeType = fetched.type; | ||
| if (mimeType && !baseName.includes('.')) { | ||
| const subtype = mimeType.split('/')[1] || ''; |
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 the feeling it is error prone, what if the mimeType does not have a "/" ?
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, good catch,
I’ve updated it so we first check for the / is present otherwise we skip the extension logic and keep the base filename
src/frontend/apps/impress/src/features/docs/doc-export/components/ModalExport.tsx
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
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]>
fbdb37c to
882b7f3
Compare
882b7f3 to
22ff23f
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