Skip to content

Comments

Moved Content to HTML functionallity to Shared API and implement IMG files#624

Merged
jgclark merged 5 commits intoNotePlan:mainfrom
cwhittl:cw-move-content-to-html-to-share-and-fix-images
Feb 20, 2025
Merged

Moved Content to HTML functionallity to Shared API and implement IMG files#624
jgclark merged 5 commits intoNotePlan:mainfrom
cwhittl:cw-move-content-to-html-to-share-and-fix-images

Conversation

@cwhittl
Copy link
Contributor

@cwhittl cwhittl commented Feb 17, 2025

Moved Content to HTML functionallity to Shared API and implement IMG files to base64

@dwertheimer dwertheimer requested a review from jgclark February 17, 2025 00:37
Copy link
Collaborator

@jgclark jgclark left a comment

Choose a reason for hiding this comment

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

Overall, makes good sense to shift the code around.
Some small points to fix, please.

* Convert a note's content to HTML and include any images as base64
* @author @cwhittl
* @param {string} content
* @param {NPNote} Note
Copy link
Collaborator

Choose a reason for hiding this comment

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

type is TNote

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated

/**
* Convert a note's content to HTML and include any images as base64
* @author @cwhittl
* @param {string} content
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why pass content and note? Is this different content from what the note has?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason is for using it with either a whole note or selected text.
I guess I could just call the editor from the function instead of passing anything?
image


/**
* Convert a note's content to HTML and include any images as base64
* @author @cwhittl
Copy link
Collaborator

Choose a reason for hiding this comment

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

Really?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You mean because i wrote a few lines it's not my code? I 100% don't know what I was thinking, removed.

* @param {string?} mermaidTheme name (optional)
*/
export function previewNote(mermaidTheme?: string): void {
export async function previewNote(mermaidTheme?: string): void {
Copy link
Collaborator

Choose a reason for hiding this comment

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

You've removed a default for mermaidTheme. Please add='green'.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@cwhittl cwhittl requested a review from jgclark February 18, 2025 13:35
@dwertheimer
Copy link
Collaborator

dwertheimer commented Feb 19, 2025

@cwhittl @jgclark I pulled and tried to test the PR but I wasn't 100% sure what to test.
I ran it on a test document I had and a bunch of things don't look quite right to me, but I'm not 100% sure what you guys would expect to be covered. I think some things (e.g. checklists etc.) may have been added since jgclark created the Preview plugin. Also some things in NotePlan are not quite standard MD so they don't look quite right.

Screen Cap 2025-02-18 at 19 19 04@2x

Here's what the @jgclark test document looks like when shared using the built-in NotePlan sharing:

Screen Cap 2025-02-18 at 20 55 22@2x

Note that Eduard is loading the fonts remotely:

Screen Cap 2025-02-18 at 20 57 10@2x

Download the test documents:
clay russell version
jgclark version here

@dwertheimer
Copy link
Collaborator

@cwhittl @jgclark I wonder if you can bake @EduardMe's custom noteplanstate font into the html itself? ChatGPT says we can.

https://chatgpt.com/share/67b55c18-1244-8012-a635-2d8f9e3c123d

Here are the font files:

noteplanstate_formats.zip

Here are the ascii chars that generate each glyph as far as I can tell:

const states = [
  { type: 'open', icon: '*', class: 'noteplanstate' },
  { type: 'done', icon: 'a', class: 'noteplanstate' },
  { type: 'scheduled', icon: 'b', class: 'noteplanstate' },
  { type: 'cancelled', icon: 'c', class: 'noteplanstate' },
  { type: 'list', icon: '-', class: 'noteplanstate' },
  { type: 'checklist', icon: '+', class: 'noteplanstate' },
  { type: 'checklistDone', icon: 'd', class: 'noteplanstate' },
  { type: 'checklistCancelled', icon: 'e', class: 'noteplanstate' },
  { type: 'checklistScheduled', icon: 'f', class: 'noteplanstate' },
]

Also in case anybody is interested for posterity, there's some Eduard -> Jonathan discussion here about FontAwesome glyphs.

@dwertheimer
Copy link
Collaborator

dwertheimer commented Feb 19, 2025

Now that I look at this document I realize... @EduardMe has gone to great lengths to make the HTML version of a published note look as much like it does in the editor as possible. Maybe you guys want to look at whether you can adapt his JS code (which is basically one JS file) to generate the HTML preview rather than using the more general purpose showdown or doing it by hand? Just a thought...

Of course, @cwhittl still needs @EduardMe to take a look at getting the HTML to clipboard.

@cwhittl
Copy link
Contributor Author

cwhittl commented Feb 19, 2025

@jgclark @dwertheimer @EduardMe

I'm really fine either way, my goal has been to be able to copy an HTML version of the a note including images to other applications. With this change to preview I can do that but completely ok switching gears and looking in another direction if you all want to point me there.

If you want I can move the code back to np.Preview and we can get the functionallity for now this way and then work on using @EduardMe version (if he's ok with that) to make a more common library.

@dwertheimer
Copy link
Collaborator

I will leave that to @jgclark. But are you able to copy to the Clipboard?

@cwhittl
Copy link
Contributor Author

cwhittl commented Feb 19, 2025

Right now to make it work I just do a preview and then copy and paste from that window. On my other plugin that I am making the clipboard doesn't work other than just Clipboard.string from what I have tested.

@EduardMe
Copy link
Contributor

Copying html into the clipboard through the API is fixed for the next update.

@jgclark
Copy link
Collaborator

jgclark commented Feb 19, 2025

IMHO we now have a couple of different design questions, and at least one bug, here.

  • 1. Bug: hopefully EM has fixed it already.
  • 2. Decide which plugin should the new functionality live in. Ideally its baked into the app itself, but until then, I think NoteHelpers is the most obvious location, as its a note-level command. Alternatively we change the name of Preview plugin to something that would naturally cover this as well. [@cwhittl FYI it's easy to change the display name of a plugin, but not the underlying pluginID.]
  • 3. How far to improve the HTML output? When I wrote Preview some while ago, I said I wasn't going to put lots of effort into perfecting the output, as @EduardMe was already working on a web version which should be able in time to give us an 'official' (and therefore maintained) HTML output function to use.

@jgclark
Copy link
Collaborator

jgclark commented Feb 19, 2025

On 2: I'm happy to try this: perhaps "HTML Output" with commands "Save as HTML" (the one one) and "Preview as HTML" (the existing one). Thoughts?

On 3: I hope @EduardMe can comment on whether getting an API call like returnHTMLForNote(note: TNote): string is likely.

@EduardMe
Copy link
Contributor

An API call like returnHTMLForNote(note: TNote): string is not planned so far.

@cwhittl
Copy link
Contributor Author

cwhittl commented Feb 20, 2025

So this PR was to make images work with Preview and to move that functionality to a shared place to be used by other plugins. Are we ok with that?

The bug that @EduardMe has put a fix for doesn't keep this PR from going it will be a problem for my extension (that's not on this PR) and will test it during that process.

@jgclark
Copy link
Collaborator

jgclark commented Feb 20, 2025

I know I was going beyond the scope of the PR with the discussion above, but wanted to capture it while I was thinking about it.
So I will now merge it.

@jgclark jgclark merged commit 470a12e into NotePlan:main Feb 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants