Not sure if we want this, but it seems to work fine.
Details
Details
- Reviewers
joshuaspence chad - Commits
- Restricted Diffusion Commit
rPbb58a123e633: Modularize Celerity postprocessors
Diff Detail
Diff Detail
- Repository
- rP Phabricator
- Branch
- theme1
- Lint
Lint Passed - Unit
Tests Passed - Build Status
Buildable 6888 Build 6910: [Placeholder Plan] Wait for 30 Seconds
Event Timeline
Comment Actions
(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.)
Comment Actions
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.
Comment Actions
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 | ||
| 8–10 | Maybe move this to CelerityPostprocessor as in PhabricatorEdgeType and PhabricatorPHIDType. | |
| src/applications/celerity/postprocessor/CelerityPostprocessor.php | ||
| 3 | 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. | |
| 31–33 | 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 | |
| 35 | 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. | |
Comment Actions
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).
Comment Actions
- 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.
