bradfitz / go-issue-mirror

[old] precursor to golang.org/x/build/maintner/godata
24 stars 2 forks source link

cmd/servegoissues: Disable reactions. #12

Closed dmitshur closed 7 years ago

dmitshur commented 8 years ago

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@c6bf1874c9ed7956a04ab06f039a8516f30d73d1).

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

bradfitz commented 8 years ago

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

dmitshur commented 8 years ago

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 commented 7 years ago

@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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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. :)