-
Notifications
You must be signed in to change notification settings - Fork 2
Conversation
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.
They're unicode code points. Why serve assets? Add a flag at least to just use Unicode? |
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., |
@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. |
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. |
All right, I'll make the other PR then, and we can close this one. After this is resolved, I'll probably remove |
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 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 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 |
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:
Screenshots
Some screenshots to visualize what this PR changes.
Before
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).
On GitHub
For reference, this is what the same issue looks like on GitHub: