Page MenuHomePhabricator

Modularize Celerity postprocessors
ClosedPublic

Authored by epriestley on Jun 19 2015, 10:06 PM.
Tags
None
Tokens
"Baby Tequila" token, awarded by chad."Evil Spooky Haunted Tree" token, awarded by joshuaspence."Love" token, awarded by tycho.tatitscheff."Like" token, awarded by btrahan.

Details

Reviewers
joshuaspence
chad
Commits
Restricted Diffusion Commit
rPbb58a123e633: Modularize Celerity postprocessors
Summary

Not sure if we want this, but it seems to work fine.

Test Plan

contrast.png (1×1 px, 201 KB)

Diff Detail

Repository
rP Phabricator
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

epriestley retitled this revision from to Modularize Celerity postprocessors.
epriestley updated this object.
epriestley edited the test plan for this revision. (Show Details)
epriestley added a reviewer: chad.
src/applications/celerity/postprocessor/CelerityHighContrastPostprocessor.php
4

You can add more postprocessors to address other visual constraints by creating more classes like this one.

20–22

You can add more overrides here. Anything not specified will use the defaults.

(I have deliberately avoided the word "theme" or any implication that this feature is fun, but I can't stop anyone from building themes with it.)

chad edited edge metadata.
This revision is now accepted and ready to land.Jun 20 2015, 7:16 AM

The only other thing I can think of is letting people pull in an actual CSS file or define their own rules, but I suppose that is actual theming and this is just to make Phabricayor easier to read.

Or that admins might want to make this a default like base language.

A couple of really nitpicky inlines. There is some inconsistency in the use of postprocessor versus postProcessor, personally I feel that the latter is more correct, but you seem to have used the former in more places.

src/applications/celerity/CelerityResourceTransformer.php
13

I feel like this method should be named setPostprocessorKey or that it should actually take an instance of CelerityPostprocessor.

src/applications/celerity/CelerityStaticResourceResponse.php
18

Maybe name this $postprocessor for consistency with CelerityResourceTransformer

36

As above, I feel that this would be more correctly named setPostprocessorKey

36–43

As above.

44–47

You maybe got a bit carried away here :P

src/applications/celerity/postprocessor/CelerityDefaultPostprocessor.php
9–11

Maybe move this to CelerityPostprocessor as in PhabricatorEdgeType and PhabricatorPHIDType.

src/applications/celerity/postprocessor/CelerityPostprocessor.php
4

Maybe add a basic test class as in D13272. I haven't seen these tests catch any real issues yet, but theoretically it means that we will catch issues with two postprocessors having the same key.

32–34

I wonder if the "return the default postprocessor" logic should live here? Like, maybe CelerityPostprocessor::getPostprocessor(null) should just return the default. Also, maybe CelerityPostprocessor::getPostprocessor('nonexistent_key') should throw an exception

36

I've been wondering if we should make these fields final? This is probably more siginificant if/when we drop PHP 5.2 support and can use late static binding.

Or that admins might want to make this a default like base language.

They can default them both after T4103 (right now, no way to choose a default language either).

epriestley edited edge metadata.
    • Use Postprocessor consistently (I started with "PostProcessor" but it looked super weird after I typed it 5 times so I switched, but didn't clean them all up).
  • Remove ocean of whitespace between tiny island methods.
  • Added tests for getAll() not hitting key collisions.
  • Marked static methods final.
  • Use postprocessorKey() when we're passing a key.
  • Fix ajax.
  • Make high contrast colors more obvious.

I also made "default to DefaultPostprocessor" a rule in the base class. This feels a little dirty (the parent refers to a child symbol by name) but simplifies subclass implementations, and the default postprocessor is more special in this case than in most cases of modular subclasses.

As an aside, I also think we could probably write some kind of shared piece of "turn these objects into a map with unique keys and throw on collision" code, which is getting more common. I think the major difference between them is specializing the strings, but I think the error is usually pretty obvious when you hit it and it would be OK to have more generic strings.

Screen Shot 2015-06-20 at 6.07.51 AM.png (1×1 px, 247 KB)

This revision was automatically updated to reflect the committed changes.