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 a Mastodon button #23

Closed
wants to merge 2 commits into from
Closed

Conversation

andybalaam
Copy link
Contributor

@andybalaam andybalaam commented Sep 23, 2020

Hi, brilliant project!

I attempted to add a Mastodon share button. It's more complicated than the others because we need to ask which Mastodon instance you use, so we need to run JavaScript when you click the link.

I implemented asking which instance using a prompt() which looks basic but does work. I also included remembering your choice so it can be the default next time. All this was inspired by https://www.256kilobytes.com/content/show/4812/ .

Let me know what you think.

Thank you for shareon, it is really cool. I am using it at https://andybalaam.gitlab.io/smolpxl/spring/

@kytta kytta self-requested a review September 23, 2020 10:27
@kytta kytta self-assigned this Sep 23, 2020
@kytta kytta added the enhancement New feature or request label Sep 23, 2020
@kytta
Copy link
Owner

kytta commented Sep 23, 2020

Hi Andy, thank you for your contribution 🙇‍♂️ I'll try to review it as soon as I can :)

Copy link
Owner

@kytta kytta left a comment

Choose a reason for hiding this comment

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

So, I have two main problems with this PR in particular: an enormous increase in file size (easily fixable) and the way Mastodon instance is selected. More in the attached comments.

@@ -16,7 +16,7 @@ const bannerText = `${pkg.name} v${pkg.version} by Nikita Karamov\n${pkg.homepag

const plugins = [
consts({
urlBuilderMap,
fullNetworkMap,
Copy link
Owner

Choose a reason for hiding this comment

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

Hmm, that ain't a good idea... So you're basically loading everything about social networks into the JS file; including the icons and the colours, that don't need to be there... That is also the reason why you had to bump the limit to 7 KB for the JS file — there is no way I can ship that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah! I didn't understand that at all, thank you!

Object.entries(NETWORKS)
.map((entry) => [
entry[0],
entry[1].url,
Copy link
Owner

@kytta kytta Sep 23, 2020

Choose a reason for hiding this comment

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

Instead of removing this whole chunk, why don't you redefine urlBuilderMap to include both url or onclick property:

const urlBuilderMap = Object.fromEntries(
  Object.entries(NETWORKS)
    .map((entry) => [
      entry[0],
      {
        url: entry[1].url,
        onclick: entry[1].onclick,
      },
    ]),
);

In shareon.js you can use both just by referencing either urlBuilderMap[cls].url or urlBuilderMap[cls].onclick

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I started out doing it like that and then thought "why not just use fullNetworkMap?" Now you have told me why :-)

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 will work on fixing this. Do you prefer a rebased PR, or additional commits on top of this one?

Copy link
Owner

Choose a reason for hiding this comment

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

Just add commits on top of this one :)

);

// eslint-disable-next-line no-alert
let instance = prompt(
Copy link
Owner

Choose a reason for hiding this comment

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

prompt is, well, fine, however, if the user earlier has clicked the "Prevent Site from Creating Anymore Dialogs" checkbox, this prompt will not appear. I don't think there should be a reason for the user not to be able to share something if the webpage nagged him with prompts or alerts before.

Perhaps it would be easier to build my own popup (similar to https://github.com/Aly-ve/Mastodon-share-button, which is also too large) or a whole statically hosted mastodon share 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.

Would you like to solve this question before you consider merging? I went for prompt because it seemed like it might be better than nothing, but if it's too ugly for you to want it, then we would need do some work.

That work might be tricky, because we don't know anything about what the rest of the page looks like, so any dialog would need to be customisable and have good defaults.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A whole statically hosted mastodon share page would be great and could have features like being integrated with joinmastodon.org to get a list of instances, plus it would remove all the complexity from this PR. But, it would be non-trivial amounts of work.

Copy link
Owner

Choose a reason for hiding this comment

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

So, I have spent a day and came up with this: https://github.com/NickKaramoff/toot. Was pretty trivial after all :)

We can generate text like we do it for WhatsApp: https://github.com/NickKaramoff/shareon/blob/bc1fa34391c9d2f99cb360587bf9322bb7cf9cef/src/networks.js#L89

And then supply it to https://toot.karamoff.dev with the text URL param. So, I envision something like this:

const NETWORKS = {
  mastodon: {
    color: '...',
    icon: '...',
    url: (d) => `https://toot.karamoff.dev/?text=${d.title}%0D%0A${d.url}${d.text ? `%0D%0A%0D%0A${d.text}` : ''}${d.via ? `%0D%0A%0D%0A${d.via}` : ''}`,
  }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wow, this is really great, and solves the problem in a much better way than I was proposing.

Copy link
Owner

@kytta kytta left a comment

Choose a reason for hiding this comment

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

Here is another improvement on the icon — it's easier for me to suggest changes and for you to commit it directly from GitHub than pulling the whole repo and committing it myself :)

src/networks.js Outdated Show resolved Hide resolved
<a class="whatsapp">Send</a>
</div>
```

## Copyright
Copy link
Owner

Choose a reason for hiding this comment

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

This is a brilliant thing that I forgot — logo copyrights! Thank you for pointing this out. I'll add an issue regarding that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah cool, thanks!

Co-authored-by: Nikita Karamov <nick@karamoff.dev>
@andybalaam
Copy link
Contributor Author

Closing in favour of #27 which does the same thing a better way.

@andybalaam andybalaam closed this Sep 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants