Page MenuHomePhabricator

JX.Favicon for Conpherence
ClosedPublic

Authored by chad on Oct 20 2016, 4:33 AM.

Details

Summary

I think maybe these should be more separate from JX.Title, but seems to work ok. May build new favicons just for messages though. Proof of concept UI.

Test Plan

Send message on one browser, see red icon in other browser. Click on menu, count and favicon switch back to normal.

Diff Detail

Repository
rP Phabricator
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

chad updated this revision to Diff 40294.Oct 20 2016, 4:33 AM
chad retitled this revision from to JX.Favicon for Conpherence.
chad updated this object.
chad edited the test plan for this revision. (Show Details)
chad added a reviewer: epriestley.
chad updated this revision to Diff 40298.Oct 20 2016, 4:18 PM
  • move javicon change to aphlict no JX.Title
epriestley requested changes to this revision.Oct 20 2016, 4:58 PM
epriestley edited edge metadata.

In many configurations, direct /rsrc/ URIs don't work (they aren't supposed to in any configuration, but I think older install instructions had users configure them to pass through (?) or apache is special (?), so they may work on some machines, but will not work on all machines). Even if they do work, they don't cache/version correctly and won't be served through a CDN properly.

So any rsrc/ URI needs to be wrapped in celerity_get_resource_uri(). If Celerity doesn't work with .ico, it needs to be made to work (not sure what the failure mode is). This also means that JS can't have a /rsrc/... URI -- you need to pass it from the server via initBehavior(..) by passing a key like messagesURI.

webroot/rsrc/js/application/aphlict/behavior-aphlict-dropdown.js
48–50

Does the red favicon get cleared if you read all your messages?

This revision now requires changes to proceed.Oct 20 2016, 4:58 PM

JX.Title and JX.Favicon should also probably just be one class, too, but we can deal with that later since both probably need a rewrite for Quicksand sooner or later anyway.

chad updated this revision to Diff 40301.Oct 20 2016, 5:23 PM
chad edited edge metadata.
  • better code?
chad added a comment.Oct 20 2016, 5:27 PM

yeah celerity barfs on .ico, I couldn't find where it checks those.

What does "barf" mean, exactly? I can probably point you in the right direction, just not sure what you're seeing.

chad added a comment.Oct 20 2016, 5:30 PM

it returns null

Try this:

  • In CelerityResourcesOnDisk->getBinaryFileSuffixes(), add .ico.
  • Run bin/celerity map.
  • In CelerityResourceController->getSupportedResourceTypes() you may need to add some fiddly magic too.
chad added a comment.Oct 20 2016, 5:57 PM

resource now gets translated, but following link 404's.... may be issue with re-write rules?

chad updated this revision to Diff 40302.Oct 20 2016, 6:05 PM
chad edited edge metadata.
  • better logic?

Hrrm, let me take a look.

chad added a comment.Oct 20 2016, 6:23 PM

yeah i can't figure this out, seems to be ico related though, pngs work fine.

D16737 seems to fix things for me locally.

One possible issue is that you should probably celerity_generate_resource_uri('rsrc/favicons/favicon.ico') to get a URI for the default icon, not just /favicon.ico. Not sure if that's really a problem, but Celerity generally only handles stuff in rsrc/.

chad updated this revision to Diff 40306.Oct 20 2016, 6:34 PM
  • updates

Working locally now?

src/applications/celerity/resources/CelerityResourcesOnDisk.php
43 ↗(On Diff #40306)

Duplicate, now?

chad updated this revision to Diff 40308.Oct 20 2016, 6:43 PM
  • extra ico
chad added a comment.Oct 20 2016, 7:11 PM

i think it's ready, seems to do what is advertised.

epriestley accepted this revision.Oct 20 2016, 7:13 PM
epriestley edited edge metadata.

goodbye huge favicons

This revision is now accepted and ready to land.Oct 20 2016, 7:13 PM
chad added a comment.Oct 20 2016, 7:17 PM

yeah, dunno what i was planning on with those. too obnoxious in practice.

they will live on forever in our hearts :)

This revision was automatically updated to reflect the committed changes.