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

Resolves #820 - Added PropertyLinks text transformer #824

Closed
wants to merge 4 commits into from

Conversation

jorgegomzar
Copy link

Description

  • added PropertyLinks text transformer to copy all links from the FrontMatter into the Markdown body so the CrawlLinks transformer can generate links for these files too.

## Context
This PR was created to address Issue #820 raised by myself.

Only tested this in my local. You can see a PoC for this change here
imagen

I am not too proficient at TS / JS so I'll gladly accept any constructive feedback / suggestions / guidelines on how to write tests.

Thanks!

@tuta-amb
Copy link
Contributor

tuta-amb commented Feb 8, 2024

you might want to try fixing some types to get the tests to pass

@aarnphm
Copy link
Collaborator

aarnphm commented Feb 8, 2024

I think this would be better not to have a additional plugin, but to add options to frontmatter and crawlinks to make this work correctly.

@jorgegomzar jorgegomzar changed the title feat: added PropertyLinks text transformer Resolves #820. Added PropertyLinks text transformer Feb 8, 2024
@jorgegomzar jorgegomzar changed the title Resolves #820. Added PropertyLinks text transformer Resolves #820 - Added PropertyLinks text transformer Feb 8, 2024
@jorgegomzar
Copy link
Author

I think this would be better not to have a additional plugin, but to add options to frontmatter and crawlinks to make this work correctly.
I thought on doing that (see the alternatives in #820) but was not sure about it.

If instead of creating this transformer I used the FrontMatter, for example, how can I add the elements to the file content?
Should I modify the tree or the vfile.text? (context)

@aarnphm
Copy link
Collaborator

aarnphm commented Feb 8, 2024

you should just resolve the links then put it to file.link. q/p/emitters/contentIndex.ts will handle setup link index correctly.

@jorgegomzar
Copy link
Author

jorgegomzar commented Feb 8, 2024

I wanted to re-use Obsidian properties' names so they will be displayed on top of the page like this:

up: <link to file>
related: <link to file 2>, <link to file 3>, <link to file 4>
another link property: <link to file 5>
---
note content ...

I get your suggestion about reusing the FrontMatter transformer, in the end I am getting the links using the same matter parser, but the issue I see is that FrontMatter runs on Markdown level and the change I want to do can be done on Text level.

So in order to display the properties and links using the FrontMatter transformer I would need to:

  • parse the properties with at least 1 link from the FrontMatter --> this is easy, already in this PR
  • append Markdown elements for each property + link using unist between the YAML properties and the file content --> trickier than just splitting the text using string methods
  • resolve the links and put them in the file.links as you suggested --> no idea about this, but I know CrawlLinks already handles this if the wiki links already exist in the file content, so I don't get why we should repeat code

It makes more sense to me to have this logic as a Text transformer prior to the Markdown and HTML transformers. It is less work and easier.

Plus, resolving the links in the FrontMatter would be repeating the logic done by the CrawlLinks HTML transformer and I don't think it is completely related to the frontmatter logic (the target of this PR is to modify the file content, not the frontmatter).

Wdyt about a middle ground? I can move to the FrontMatter transformer the logic that appends the properties with links between the YAML and the note content, but not resolve the links since they will be in the file.value and CrawlLinks already handles that.

@aarnphm
Copy link
Collaborator

aarnphm commented Feb 8, 2024

there is a textTransform.

@jorgegomzar
Copy link
Author

jorgegomzar commented Feb 8, 2024

there is a textTransform.

So, instead of creating a new textTransform should I move it to the ObsidianFlavoredMarkdown tansformer?

EDIT: pushed a commit with these changes, let me know if I need to change anything else

Comment on lines +20 to +22
import yaml from "js-yaml"
import toml from "toml"
import matter from "gray-matter"
Copy link
Collaborator

Choose a reason for hiding this comment

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

You actually don't have to do any of these parsing.

Probably will only need to parse the links within markdownPlugins.

you can get the frontmatter from file.frontmatter.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This means the nested frontmatter links would also getting parsed afaik.

probably should set the options by default.

@aarnphm
Copy link
Collaborator

aarnphm commented Feb 9, 2024

Also, just a heads up: We will do a refactor soon, so I'm going to close the PR.

See https://discord.com/channels/927628110009098281/1149197588084572161/1205045486596268032 for more information.

@aarnphm aarnphm closed this Feb 9, 2024
@jorgegomzar
Copy link
Author

Gotcha, will keep an eye then. Thanks for your time

@robinsving
Copy link

Since the link to the Discord is broken, I'll ask here: What is the status of the attached Feature request?

@kireevys
Copy link

I wait this features too

@0atman
Copy link

0atman commented Oct 28, 2024

Just a note, this PR still applies cleanly on my current Quartz install (installed TODAY).

If anyone wants this feature now (maybe @robinsving or @kireevys), you can apply it, or simply copy and paste the lines into your quartz/plugins/transformers/ofm.ts file, as of today 🎉

@saberzero1
Copy link
Collaborator

Just a note, this PR still applies cleanly on my current Quartz install (installed TODAY).

If anyone wants this feature now (maybe @robinsving or @kireevys), you can apply it, or simply copy and paste the lines into your quartz/plugins/transformers/ofm.ts file, as of today 🎉

Oh hey, you're the YouTuber that sent me down the NixOS rabbithole. Love your videos.

Anyway, welcome to Quartz.

We're currently reworking the transformers to improve configurability and extensibility. I'll add frontmatter link parsing to the list of default Obsidian features to account for.

Feel free to reach out on the Quartz Discord server if you have any questions or need assistance with configuration.

@0atman
Copy link

0atman commented Oct 28, 2024

Thank you so much @saberzero1, I'll try out those transformers when they're released.

I don't know if I should apologise or not for sending you down the nixos rabbithole, but thank you for your nice words! 😃

I am wary of the discord walled garden for this sort of thing, but I appreciate it's where a lot of conversations happen. Will do if I need help, though the documentation is LOVELY.

@saberzero1
Copy link
Collaborator

Thank you so much @saberzero1, I'll try out those transformers when they're released.

I don't know if I should apologise or not for sending you down the nixos rabbithole, but thank you for your nice words! 😃

I am wary of the discord walled garden for this sort of thing, but I appreciate it's where a lot of conversations happen. Will do if I need help, though the documentation is LOVELY.

Haha. It's been great.

The Discord server is mostly used for user-specific requests/questions or assistance in troubleshooting or customizations beyond the default options included in the documentation. Based on your YouTube videos, you probably fall on the more customization-heavy side. 😇

For general things we prefer to use GitHub (issues, bug tracking, feature requests, etc)

@0atman
Copy link

0atman commented Oct 28, 2024

Nice, I like the force multiplier of public discourse 👌

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.

7 participants