Skip to content
This repository was archived by the owner on Sep 17, 2019. It is now read-only.

cmd/servegoissues: Disable reactions. #12

Closed
wants to merge 1 commit into from
Closed

cmd/servegoissues: Disable reactions. #12

wants to merge 1 commit into from

Conversation

dmitshur
Copy link
Collaborator

@dmitshur dmitshur commented Oct 15, 2016

The new issuesapp API (see shurcooL/issuesapp#1) requires the user to provide emojis (they are now served locally rather than from the internet), unless DisableReactions option is true (it's available as of shurcooL/issuesapp@c6bf187).

Given that the Go issues data does not include any reactions, and it's not possible to login to react, it seems to make more sense to outright remove them.

@bradfitz Let me know if that's what you prefer to do. The alternative is to not disable reactions. But then you'll need to serve emojis locally to avoid 404s if users open the "React" menu. It would be a matter of adding:

import "github.com/shurcooL/reactions/emojis"
emojisHandler := httpgzip.FileServer(emojis.Assets, httpgzip.FileServerOptions{})
http.Handle("/emojis/", http.StripPrefix("/emojis", emojisHandler))

Screenshots

Some screenshots to visualize what this PR changes.

Before

image

After

Notice the "React" button in top right corner of each comment is now hidden. All reactions, if they were present in the underlying data, are hidden too (but there are none included anyway).

image

On GitHub

For reference, this is what the same issue looks like on GitHub:

image

The new issuesapp API (see shurcooL/issuesapp#1) requires the user to
provide emojis (they are now served locally rather than from the
internet), unless DisableReactions is true.

Given that the Go issues data does not include any reactions, and it's
not possible to login to react, it seems to make more sense to outright
remove them.
@bradfitz
Copy link
Owner

They're unicode code points. Why serve assets? Add a flag at least to just use Unicode?

@dmitshur
Copy link
Collaborator Author

dmitshur commented Oct 15, 2016

Reaction images are currently implemented as a .png texture atlas (e.g., see here).

Unicode characters require a font that supports emoji, and such a font isn't available or is highly inconsistent on different platforms. I don't want the same reaction to look wildly different depending on where you're viewing it.

Also, reactions are not simply unicode characters. They're a sorted mapping of reaction names (e.g., :+1:, :grinning:, :mostly_sunny:, :stuck_out_tongue_winking_eye:, :raised_hands::skin-tone-4:, etc.) to emoji (which can be generally represented as unicode characters, yes, but not if it's a custom emoji that doesn't have a unicode character, or if skin-tones are used).

@dmitshur
Copy link
Collaborator Author

dmitshur commented Oct 22, 2016

@bradfitz, should I interpret your response to mean that you'd like to preserve reactions? What for, given they're currently not used (i.e., they're stripped from the upstream GitHub data)?

If you'd like, I can make a PR that preserves support for displaying them, but emoji image data assets will be required for that.

@bradfitz
Copy link
Owner

Yes, I'd like to have reactions in the data. Seems like a useful signal, especially considering we tell people to use them instead of +1 comments.

@dmitshur
Copy link
Collaborator Author

All right, I'll make the other PR then, and we can close this one.

After this is resolved, I'll probably remove DisableReactions option since I made it specifically for this PR, and I don't expect anyone else to use it.

@dmitshur dmitshur closed this Oct 24, 2016
@dmitshur
Copy link
Collaborator Author

dmitshur commented Oct 24, 2016

Yes, I'd like to have reactions in the data.

You'll need to make a change in mirrorissues.go#L170 and mirrorissues.go#L204 to preserve the reactions data.

Each reaction entry from GitHub API looks like this:

{
  "id": 4459856,
  "user": {
    "login": "shurcooL-testing",
    "id": 17713359,
    "avatar_url": "https://avatars.githubusercontent.com/u/17713359?v=3",
    "gravatar_id": "",
    "url": "https://api.github.com/users/shurcooL-testing",
    "html_url": "https://github.com/shurcooL-testing",
    "followers_url": "https://api.github.com/users/shurcooL-testing/followers",
    "following_url": "https://api.github.com/users/shurcooL-testing/following{/other_user}",
    "gists_url": "https://api.github.com/users/shurcooL-testing/gists{/gist_id}",
    "starred_url": "https://api.github.com/users/shurcooL-testing/starred{/owner}{/repo}",
    "subscriptions_url": "https://api.github.com/users/shurcooL-testing/subscriptions",
    "organizations_url": "https://api.github.com/users/shurcooL-testing/orgs",
    "repos_url": "https://api.github.com/users/shurcooL-testing/repos",
    "events_url": "https://api.github.com/users/shurcooL-testing/events{/privacy}",
    "received_events_url": "https://api.github.com/users/shurcooL-testing/received_events",
    "type": "User",
    "site_admin": false
  },
  "content": "hooray",
  "created_at": "2016-10-24T02:23:03Z"
}

I recommend cleaning all fields except content, and user.login. Those are the only two that are really needed for presentation purposes.

After that's done, we'll need to make a change to preserve/process reactions at servegoissues.go#L228 and servegoissues.go#L238.

I've also realized you won't need all of github.com/shurcooL/reactions/emojis.Assets because GitHub only has 6 reactions (not 1000+ that issuesapp supports) and that's all that needs to be supported here.

I will let you lead the effort here and set the pace and priority. Whenever you think this is worth doing, get that first step done, and I'll follow it up with the rest needed to get issuesapp to display the reactions. I'm not in a rush to get this done, so it's completely up to you. :)

@dmitshur dmitshur deleted the servegoissues-disable-reactions branch October 24, 2016 02:40
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants