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

Feature: Description of timers and projects parsed as Markdown #122

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mihaibirsan
Copy link
Contributor

@mihaibirsan mihaibirsan commented Mar 31, 2023

Feature

Interpret the description of time entries as Markdown. This enables using the description to link to project notes.

Before:
Pasted image 20230331153910

After:
Pasted image 20230331153945

Remaining work

Completed partially

  • Add toggle in settings; default behavior should be not to interpret as Markdown (compatibility for existing users)
  • Consider parsing project names as well
  • Consider limiting markdown parsing to links only
  • Update tests

@mihaibirsan mihaibirsan changed the title WIP: Feature: Markdown in time entry's name Description of timers and projects parsed as Markdown May 13, 2023
@mihaibirsan mihaibirsan force-pushed the feat-time-entry-markdown-name branch from c019aae to 179fa57 Compare May 13, 2023 18:51
@mihaibirsan mihaibirsan changed the title Description of timers and projects parsed as Markdown Feature: Description of timers and projects parsed as Markdown May 13, 2023
@mihaibirsan
Copy link
Contributor Author

This is technically ready to ship.

Considered out of scope:

  • Consider limiting markdown parsing to links only
    • Using Obsidian's own parser, which doesn't allow for much customization; when this becomes a real user issue, we may add more granular control over the markdown parser (and even consider discarding the innerHTML trick)
  • Update tests
    • Codebase doesn't have UI tests at this time (perhaps open an issue?)

@mihaibirsan mihaibirsan force-pushed the feat-time-entry-markdown-name branch from 179fa57 to f8f6593 Compare May 15, 2023 15:10
@mihaibirsan
Copy link
Contributor Author

Published a pre-release version with all 2 features and 1 fix currently in PR!

See 0.10.0+87c4156 for instructions on how to test!

@mcndt
Copy link
Owner

mcndt commented Jul 23, 2023

Cool feature!

IMO I think this is a very niche option, as most users probably don't want raw Markdown syntax in their time entries (since it's not rendered anywhere outside of Obsidian)

But I don't see any harm in releasing this since it's opt-in. I'll release it after doing some further testing.

Copy link
Owner

@mcndt mcndt left a comment

Choose a reason for hiding this comment

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

There's still some minor concerns here, let me know if you want to iterate.


export const renderMarkdown = (markdown: string) => {
const el = document.createElement("span");
MarkdownPreviewView.renderMarkdown(markdown, el, "", null);
Copy link
Owner

Choose a reason for hiding this comment

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

I get the following console error when loading the plugin:

image

Probably related to the null argument here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch! I'll have to find a way to pass the reference through the context, I think.

<span class="timer-description">{timer.description}</span>
<span class="timer-description">
{#if $settingsStore.parseMarkdown}
{@html renderMarkdown(timer.description)}
Copy link
Owner

Choose a reason for hiding this comment

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

This correctly renders the Markdown as a link, but clicking the link doesn't actually open the related page.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's odd. I'll investigate.

<span>
<span>
{#if $settingsStore.parseMarkdown}
{@html d.title ? renderMarkdown(d.title) : "(No project)"}
Copy link
Owner

Choose a reason for hiding this comment

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

I'm not sure if project titles should also be parsed as Markdown, only time entries make sense I think.

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 agree, I'll revert this bit and experiment with the suggestion below.

@mcndt
Copy link
Owner

mcndt commented Jul 23, 2023

I have thought before to link projects to pages in the opposite direction, namely using the frontmatter of a note we can store to which Toggl project it is related. I think that makes the UX a little better for non-power users like yourself, what are your thoughts?

@mihaibirsan
Copy link
Contributor Author

Thanks for the review @mcndt! I had checked out for the past six months, focused on work. 😅

Linking projects to pages in the opposite direction makes sense! I can prototype something with it separately. I hadn't thought about that before.

I also didn't like how it made the Toggl interface less usable. (And tinkering with user scripts to parse Markdown everywhere turned out to be a pointless task.)

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.

2 participants