Page MenuHomePhabricator

JX.Favicon for Conpherence
ClosedPublic

Authored by chad on Oct 20 2016, 4:33 AM.
Tags
None
Referenced Files
F12832676: D16734.id40298.diff
Thu, Mar 28, 1:33 PM
Unknown Object (File)
Thu, Mar 28, 1:13 AM
Unknown Object (File)
Sun, Mar 24, 1:12 AM
Unknown Object (File)
Tue, Mar 5, 1:23 AM
Unknown Object (File)
Feb 14 2024, 12:19 PM
Unknown Object (File)
Feb 14 2024, 12:19 PM
Unknown Object (File)
Feb 14 2024, 12:17 PM
Unknown Object (File)
Feb 14 2024, 12:17 PM
Subscribers

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
Branch
favicon (branched from master)
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 14194
Build 18446: Run Core Tests
Build 18445: arc lint + arc unit

Event Timeline

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.
  • move javicon change to aphlict no JX.Title
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 edited edge metadata.
  • better code?

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.

Try this:

  • In CelerityResourcesOnDisk->getBinaryFileSuffixes(), add .ico.
  • Run bin/celerity map.
  • In CelerityResourceController->getSupportedResourceTypes() you may need to add some fiddly magic too.

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

chad edited edge metadata.
  • better logic?

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/.

Working locally now?

src/applications/celerity/resources/CelerityResourcesOnDisk.php
42–43

Duplicate, now?

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

epriestley edited edge metadata.

goodbye huge favicons

This revision is now accepted and ready to land.Oct 20 2016, 7:13 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.