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

Add follow function #33

Merged
merged 1 commit into from
Jul 15, 2018
Merged

Add follow function #33

merged 1 commit into from
Jul 15, 2018

Conversation

mdbraber
Copy link
Contributor

As I often want to open links directly I added a "follow" function to open the current link under the cursor

@mgsloan
Copy link
Owner

mgsloan commented Jul 15, 2018

Good idea, thanks for the PR!

Unfortunately, I forgot about the notification for this PR when I pushed out version 16, but it will be included in version 17 of the extension.

For future contributions if'd be ideal if you ran ./eslint.sh and resolved the issues it reports. I don't mind fixing it though (I wish more maintainers would just fix things they want changed in PRs instead of fixing it themselves - seems more efficient!)

It would also be good to update readme.md and changelog.md, but I'll do that too.

@mgsloan mgsloan merged commit bc60f3e into mgsloan:master Jul 15, 2018
@mgsloan
Copy link
Owner

mgsloan commented Jul 15, 2018

      var parser = new DOMParser();
      var pc = parser.parseFromString(content.innerHTML, 'text/html');
      var link = pc.getElementsByClassName('ex_link')[0].getAttribute('href');

BTW it's totally redundant and potentially error prone to parse innerHTML. Is there a reason for doing this? I've replaced it with getFirstClass(content, 'ex_link'), but yeah I wouldn't expect people to know about the little utility library that has things like this :)

I'm also wondering about

      if (!link.match(/^https?:\/\//i)) {
        link = 'http://' + link;
      }

I will leave this in, but I'd be surprised if it is necessary - I'd think that window.open could handle any thing href can, except perhaps relative links, and this url modification code would break relative links. Not that relative urls would be very useful in this context.

@mgsloan
Copy link
Owner

mgsloan commented Jul 15, 2018

Also, it's much better to just click the link. This way the browser will take into account the target=_blank attribute, and open a new tab instead of a whole new window. So now the main body of the implementation is quite simple:

      var link = getFirstClass(content, 'ex_link');
      if (link) {
        click(link);
      }

Due to this link clicking implementation I have also removed the URL munging code.

mgsloan added a commit that referenced this pull request Jul 15, 2018
mgsloan added a commit that referenced this pull request Jul 15, 2018
mgsloan added a commit that referenced this pull request Jul 15, 2018
@mgsloan
Copy link
Owner

mgsloan commented Jul 26, 2018

I use this all the time now - great idea!

@mgsloan
Copy link
Owner

mgsloan commented Jul 26, 2018

I've released this, it is now included in version 17 👍

@mdbraber
Copy link
Contributor Author

Great! (Honestly I've transitioned back to Things 3, but it's good to hear this is being used :-)

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

2 participants