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

Enhancement: Clicking a PDF annotation link now causes an immediate jump to the annotation in markdown-source-view #258

Merged
merged 8 commits into from Jan 5, 2023

Conversation

HardwayLinka
Copy link
Contributor

@HardwayLinka HardwayLinka commented Nov 14, 2022

Hey, @aladmit, @jonasmerlin, @elias-sundqvist and @rdimaio
Deleted resources/cdn.hypothes.is/hypothesis/build on the basis of #257

ps. Use main.js and manifest.json in 0.2.7patched to test without building the project

@HardwayLinka HardwayLinka changed the title Clean build Clicking a PDF annotation link now causes an immediate jump to the annotation in markdown-source-view Nov 14, 2022
@HardwayLinka HardwayLinka changed the title Clicking a PDF annotation link now causes an immediate jump to the annotation in markdown-source-view Enhancement: Clicking a PDF annotation link now causes an immediate jump to the annotation in markdown-source-view Nov 15, 2022
@elias-sundqvist
Copy link
Owner

link click example

It looks like clicking links in the markdown source view of the annotation note still does not work. Is this supposed to be covered by the PR?

Some other notes:

  • Code looks pretty good, but it could be nice if you moved some of the logic to a separate file so that main.tsx is less cluttered.
  • What was the reason for removing this.registerEvent and handling the events manually? I believe the purpose of this.registerEvent is that the events will be removed when the plugin is unloaded.
  • I would have preferred if the solution did not use mutation observers, but it is not a deal breaker.

@HardwayLinka
Copy link
Contributor Author

HardwayLinka commented Nov 28, 2022

Hey @elias-sundqvist, thank you for your reply. This PR doesn't cover clicking links in the markdown source view of the annotation note.

1.Code looks pretty good, but it could be nice if you moved some of the logic to a separate file so that main.tsx is less cluttered.

I moved some of the logic from src/main.tsx to src/sourceViewObserver.tsx

2.What was the reason for removing this.registerEvent and handling the events manually? I believe the purpose of this.registerEvent is that the events will be removed when the plugin is unloaded.

bingo

3.I would have preferred if the solution did not use mutation observers, but it is not a deal breaker.

I can't think of a solution other than useing mutation observers

test file here

@elias-sundqvist
Copy link
Owner

To clarify on 2. I think this is already done automatically by this.registerEvent, so I don't think that part of the PR is needed?

@HardwayLinka
Copy link
Contributor Author

Hey @elias-sundqvist, I deleted handling the events manually

@aladmit
Copy link
Collaborator

aladmit commented Jan 5, 2023

It looks like @HardwayLinka resolved all concerns, so I'm going to merge it and release as 0.2.9 🚀

@aladmit aladmit merged commit b405940 into elias-sundqvist:master Jan 5, 2023
const activeLeaf = this.plugin.app.workspace.getActiveViewOfType(MarkdownView);
if (!activeLeaf) return;

const regex2Find = /(?<=[\[]{2})[^\]]*(?=[\]]{2})/gm;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Fun fact. Plugin didn't start on iOS(#284) because of this line🤡

Turned out it because Safari doesn't support ?<= and throws syntax error. I didn't find other way to exclude [[ with regex and added removing it with hrefLink.substring(2).

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.

None yet

3 participants