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

Feat #879: Different embedding styles prep #1273

Merged
merged 20 commits into from
Aug 23, 2023
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
4e2f07a
Refactor note embedding to be extensible
badsketch Aug 18, 2023
e05c1c2
Refactor pattern to use more functional approach
badsketch Aug 20, 2023
33e4438
Simplify extractor formatter invocation
badsketch Aug 20, 2023
e3fe400
Prepare new setting to replace preview.embedNoteInContainer
badsketch Aug 20, 2023
3b1c903
Fix wikilink-embed e2e tests
badsketch Aug 20, 2023
7d97e78
Improve readability
badsketch Aug 22, 2023
2598689
Try to fix wikilink-embed e2e tests by returning md content instead o…
badsketch Aug 22, 2023
35536c5
Revert "Try to fix wikilink-embed e2e tests by returning md content i…
badsketch Aug 22, 2023
c1eaded
Try to fix wikilink-embed e2e tests by resetting mocks in unit tests
badsketch Aug 22, 2023
f86dc7e
Try to fix wikilink-embed e2e tests by deleting unit test
badsketch Aug 22, 2023
7caac91
Try to fix wikilink-embed e2e tests by removing extra await
badsketch Aug 22, 2023
17baa5c
Try to fix wikilink-embed e2e tests by deleting the e2e tests to chec…
badsketch Aug 22, 2023
f4e8575
Try to fix wikilink-embed e2e tests by recreating the e2e tests to ch…
badsketch Aug 22, 2023
2f5fe5e
Try to fix wikilink-embed e2e tests by restoring deleted spacing
badsketch Aug 22, 2023
4f773f9
Try to fix wikilink-embed e2e tests by removing embedNoteInContainer …
badsketch Aug 23, 2023
24a6b59
Revert "Try to fix wikilink-embed e2e tests by removing embedNoteInCo…
badsketch Aug 23, 2023
4190c43
Try to fix wikilink-embed e2e tests by fixing the name of the config …
badsketch Aug 23, 2023
090d941
Try to fix wikilink-embed e2e tests by initializing embedNoteInContai…
badsketch Aug 23, 2023
7b9c95d
Revert "Try to fix wikilink-embed e2e tests by deleting unit test"
badsketch Aug 23, 2023
7365f5d
Set embedNoteInContainer to null in e2e tests to more closely mimic l…
badsketch Aug 23, 2023
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
15 changes: 14 additions & 1 deletion packages/foam-vscode/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -514,7 +514,20 @@
"foam.preview.embedNoteInContainer": {
"type": "boolean",
"default": true,
"description": "Wrap embedded notes in a container when displayed in preview panel"
"description": "Wrap embedded notes in a container when displayed in preview panel",
"deprecationMessage": "*DEPRECATED* use foam.preview.embedNoteType instead."
},
"foam.preview.embedNoteType": {
"type": "string",
"default": "full-card",
"enum": [
"full-inline",
"full-card"
riccardoferretti marked this conversation as resolved.
Show resolved Hide resolved
],
"enumDescriptions": [
"Include the section with title and style inline",
"Include the section with title and style it within a container"
]
},
"foam.graph.titleMaxLength": {
"type": "number",
Expand Down
18 changes: 9 additions & 9 deletions packages/foam-vscode/src/features/preview/wikilink-embed.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import {
} from '../../test/test-utils-vscode';
import {
default as markdownItWikilinkEmbed,
CONFIG_EMBED_NOTE_IN_CONTAINER,
CONFIG_EMBED_NOTE_TYPE,
} from './wikilink-embed';

const parser = createMarkdownParser();
Expand All @@ -21,8 +21,8 @@ describe('Displaying included notes in preview', () => {
]);
const ws = new FoamWorkspace().set(parser.parse(note.uri, note.content));
await withModifiedFoamConfiguration(
CONFIG_EMBED_NOTE_IN_CONTAINER,
false,
CONFIG_EMBED_NOTE_TYPE,
'full-inline',
() => {
const md = markdownItWikilinkEmbed(MarkdownIt(), ws, parser);

Expand All @@ -48,8 +48,8 @@ describe('Displaying included notes in preview', () => {
const ws = new FoamWorkspace().set(parser.parse(note.uri, note.content));

await await withModifiedFoamConfiguration(
CONFIG_EMBED_NOTE_IN_CONTAINER,
true,
CONFIG_EMBED_NOTE_TYPE,
'full-container',
() => {
const md = markdownItWikilinkEmbed(MarkdownIt(), ws, parser);

Expand Down Expand Up @@ -83,8 +83,8 @@ This is the third section of note E
const md = markdownItWikilinkEmbed(MarkdownIt(), ws, parser);

await withModifiedFoamConfiguration(
CONFIG_EMBED_NOTE_IN_CONTAINER,
false,
CONFIG_EMBED_NOTE_TYPE,
'full-inline',
() => {
expect(
md.render(`This is the root node.
Expand Down Expand Up @@ -120,8 +120,8 @@ This is the third section of note E
const ws = new FoamWorkspace().set(parser.parse(note.uri, note.content));

await withModifiedFoamConfiguration(
CONFIG_EMBED_NOTE_IN_CONTAINER,
true,
CONFIG_EMBED_NOTE_TYPE,
'full-container',
() => {
const md = markdownItWikilinkEmbed(MarkdownIt(), ws, parser);

Expand Down
28 changes: 28 additions & 0 deletions packages/foam-vscode/src/features/preview/wikilink-embed.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
import { retrieveNoteConfig } from './wikilink-embed';
import * as config from '../../services/config';

describe('Wikilink Note Embedding', () => {
describe('Config Parsing', () => {
it('should use preview.embedNoteType if deprecated preview.embedNoteInContainer not used', () => {
jest
.spyOn(config, 'getFoamVsCodeConfig')
.mockReturnValueOnce('full-card')
.mockReturnValueOnce(false);

const { noteScope, noteStyle } = retrieveNoteConfig();
expect(noteScope).toEqual('full');
expect(noteStyle).toEqual('card');
});

it('should use preview.embedNoteInContainer if set', () => {
jest
.spyOn(config, 'getFoamVsCodeConfig')
.mockReturnValueOnce('full-inline')
.mockReturnValueOnce(true);

const { noteScope, noteStyle } = retrieveNoteConfig();
expect(noteScope).toEqual('full');
expect(noteStyle).toEqual('card');
});
});
});
100 changes: 81 additions & 19 deletions packages/foam-vscode/src/features/preview/wikilink-embed.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import { Position } from '../../core/model/position';
import { TextEdit } from '../../core/services/text-edit';

export const CONFIG_EMBED_NOTE_IN_CONTAINER = 'preview.embedNoteInContainer';
export const CONFIG_EMBED_NOTE_TYPE = 'preview.embedNoteType';
const refsStack: string[] = [];

export const markdownItWikilinkEmbed = (
Expand Down Expand Up @@ -45,27 +46,31 @@ export const markdownItWikilinkEmbed = (
return `<div class="foam-cyclic-link-warning">Cyclic link detected for wikilink: ${wikilink}</div>`;
}
let content = `Embed for [[${wikilink}]]`;
let html: string;

switch (includedNote.type) {
case 'note': {
let noteText = readFileSync(includedNote.uri.toFsPath()).toString();
const section = Resource.findSection(
includedNote,
includedNote.uri.fragment
);
if (isSome(section)) {
const rows = noteText.split('\n');
noteText = rows
.slice(section.range.start.line, section.range.end.line)
.join('\n');
let { noteScope, noteStyle } = retrieveNoteConfig();
riccardoferretti marked this conversation as resolved.
Show resolved Hide resolved

let extractor: EmbedNoteExtractor = fullExtractor;
switch (noteScope) {
case 'full':
extractor = fullExtractor;
break;
}

let formatter: EmbedNoteFormatter = cardFormatter;
switch (noteStyle) {
case 'card':
formatter = cardFormatter;
break;
case 'inline':
formatter = inlineFormatter;
break;
Copy link
Collaborator

Choose a reason for hiding this comment

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

a more condensed way to achieve this (we use it in other places in the Foam codebase):

const formatter = noteStyle === 'card'
  ? cardFormatter
  : noteStyle === ' inline'
  ? inlineFormatter
  : cardFormatter // your default

It's purely a matter of style, for me the advantage is that it allows to use const (a bummer that js/ts don't have a functional switch statement), it's a bit more clear that the conditions are just about initiating the variable, and that the conditions can a bit more flexible. I think it takes a sec getting used to, but it flows once the eye is trained :)

For consistency with the codebase I would lean towards using the pattern for both extractor and formatter.

see link-completion.ts:188 and generate-link-references.ts:68

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm all for consistency! I can't help but unravel ternary operators in my head, which make chained ones seem complex, but you're right, they start looking just like an if/else block in time. Updated!

}
noteText = withLinksRelativeToWorkspaceRoot(
noteText,
parser,
workspace
);
content = getFoamVsCodeConfig(CONFIG_EMBED_NOTE_IN_CONTAINER)
? `<div class="embed-container-note">${md.render(noteText)}</div>`
: noteText;

content = extractor(includedNote, parser, workspace);
html = formatter(content, md);
break;
}
case 'attachment':
Expand All @@ -74,14 +79,15 @@ export const markdownItWikilinkEmbed = (
${md.renderInline('[[' + wikilink + ']]')}<br/>
Embed for attachments is not supported
</div>`;
html = md.render(content);
break;
case 'image':
content = `<div class="embed-container-image">${md.render(
`![](${md.normalizeLink(includedNote.uri.path)})`
)}</div>`;
html = md.render(content);
break;
}
const html = md.render(content);
refsStack.pop();
return html;
} catch (e) {
Expand Down Expand Up @@ -123,4 +129,60 @@ function withLinksRelativeToWorkspaceRoot(
return text;
}

export function retrieveNoteConfig(): {
noteScope: string;
noteStyle: string;
} {
let config = getFoamVsCodeConfig<string>(CONFIG_EMBED_NOTE_TYPE); // ex. full-inline
let [noteScope, noteStyle] = config.split('-');

// **DEPRECATED** setting to be removed
// for now it overrides the above to preserve user settings if they have it set
if (getFoamVsCodeConfig<boolean>(CONFIG_EMBED_NOTE_IN_CONTAINER, false)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

From this I assume that if someone has both settings, the old setting will overwrite the new one?
I think that the new setting should always have the precedence, and we should only read the previous one if the new setting has not been used, wdyt?

noteStyle = 'card';
}
return { noteScope, noteStyle };
}

badsketch marked this conversation as resolved.
Show resolved Hide resolved
/**
* A type of function that gets the desired content of the note
*/
export type EmbedNoteExtractor = (
note: Resource,
parser: ResourceParser,
workspace: FoamWorkspace
) => string;

function fullExtractor(
note: Resource,
parser: ResourceParser,
workspace: FoamWorkspace
): string {
let noteText = readFileSync(note.uri.toFsPath()).toString();
const section = Resource.findSection(note, note.uri.fragment);
if (isSome(section)) {
const rows = noteText.split('\n');
noteText = rows
.slice(section.range.start.line, section.range.end.line)
.join('\n');
}
noteText = withLinksRelativeToWorkspaceRoot(noteText, parser, workspace);
return noteText;
}

/**
* A type of function that renders note content with the desired style in html
*/
export type EmbedNoteFormatter = (content: string, md: markdownit) => string;

function cardFormatter(content: string, md: markdownit): string {
return md.render(
`<div class="embed-container-note">${md.render(content)}</div>`
);
}

function inlineFormatter(content: string, md: markdownit): string {
return md.render(content);
}

export default markdownItWikilinkEmbed;
Loading