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

Desktop: Resolves #7612 : Fixing Note Viewer theme reset issue on switching HTML to Md note or between 2 HTML notes. #7708

Closed
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
Fix for improper HTML Rendering style upon note switching
  • Loading branch information
deepampriyadarshi committed Feb 24, 2023
commit 0520a8d0eb2c013cb2e7d1d05cd6343445197d37
Original file line number Diff line number Diff line change
Expand Up @@ -671,6 +671,7 @@ function CodeMirror(props: NoteBodyEditorProps, ref: any) {
if (!webviewReady) return;

const options: any = {
noteId: props.noteId,
pluginAssets: renderedBody.pluginAssets,
downloadResources: Setting.value('sync.resourceDownloadMode'),
markupLineCount: editorRef.current?.lineCount() || 0,
Expand Down
29 changes: 21 additions & 8 deletions packages/app-desktop/gui/note-viewer/index.html
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,7 @@
}

let pluginAssetsAdded_ = {};
let currentNoteId = ''

try {
const contentElement = document.getElementById('joplin-container-content');
Expand Down Expand Up @@ -154,15 +155,25 @@
setPercentScroll(percentScroll_);
}

// Note that this function keeps track of what's been added so as not to
// add the same CSS files multiple times.
function addPluginAssets(assets) {
// This function now keeps a track of the css or javascript files loaded as well switching of notes.
// Recreating of div#joplin-container-pluginAssetsContainer on loading a new note ensures that plugin
// assets belonging to current note are only loaded into view to avoid css styling conflict issue.
function addPluginAssets(noteId, assets) {
if (!assets) return;

const pluginAssetsContainer = document.getElementById('joplin-container-pluginAssetsContainer');

const processedAssetIds = [];

let pluginAssetsContainer = document.getElementById('joplin-container-pluginAssetsContainer');

if (currentNoteId !== noteId) {
pluginAssetsContainer.remove();
currentNoteId = noteId;
pluginAssetsAdded_ = {};
const joplinContainerBody = document.getElementById('joplin-container-body');
const newPluginAssetsContainer = document.createElement('div');
newPluginAssetsContainer.id = 'joplin-container-pluginAssetsContainer';
joplinContainerBody.insertBefore(newPluginAssetsContainer, joplinContainerBody.firstChild);
pluginAssetsContainer = newPluginAssetsContainer;
}

for (let i = 0; i < assets.length; i++) {
const asset = assets[i];

Expand Down Expand Up @@ -385,7 +396,9 @@
setPercentScroll(event.options.percent);
}

addPluginAssets(event.options.pluginAssets);
if (currentNoteId === '') currentNoteId = event.options.noteId

addPluginAssets(event.options.noteId, event.options.pluginAssets);

if (event.options.downloadResources === 'manual') {
webviewLib.setupResourceManualDownload();
Expand Down
10 changes: 5 additions & 5 deletions packages/renderer/MdToHtml.ts
Original file line number Diff line number Diff line change
Expand Up @@ -378,12 +378,13 @@ export default class MdToHtml {
}

private async outputAssetsToExternalAssets_(output: any) {
for (const cssString of output.cssStrings) {
const newOutput = { ...output, pluginAssets: output.pluginAssets.slice() };
for (const cssString of newOutput.cssStrings) {
const filePath = await this.fsDriver().cacheCssToFile(cssString);
output.pluginAssets.push(filePath);
newOutput.pluginAssets.push(filePath);
}
delete output.cssStrings;
return output;
delete newOutput.cssStrings;
return newOutput;
Copy link
Owner

Choose a reason for hiding this comment

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

Thanks for updating this. Any change to add a test that would prove that modifying the output of this function doesn't modify the cached value?

Copy link
Author

Choose a reason for hiding this comment

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

Yeah sure. There's a test file packages/app-cli/tests/MdToHtml.ts, should I make the necessary changes in that? Other than this there's no other test file related to MdToHtml.ts. I guess this is the test file that runs on running yarn test from root directory.

Copy link
Owner

Choose a reason for hiding this comment

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

Yes this file sounds good

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for updating this. Any change to add a test that would prove that modifying the output of this function doesn't modify the cached value?

Done 👍

}

// The string we are looking for is: <p></p>\n
Expand Down Expand Up @@ -582,7 +583,6 @@ export default class MdToHtml {
let cssStrings = noteStyle(options.theme, {
contentMaxWidth: options.contentMaxWidth,
});

let output = { ...this.allProcessedAssets(allRules, options.theme, options.codeTheme) };
cssStrings = cssStrings.concat(output.cssStrings);

Expand Down