Page MenuHomePhabricator

Show configurable level in the Document Hierarchy in Phriction.
Needs RevisionPublic

Authored by firejdl on Jan 24 2014, 10:05 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Mar 16, 12:19 PM
Unknown Object (File)
Fri, Mar 1, 6:12 AM
Unknown Object (File)
Feb 21 2024, 2:45 PM
Unknown Object (File)
Feb 13 2024, 7:45 PM
Unknown Object (File)
Feb 1 2024, 11:53 PM
Unknown Object (File)
Jan 22 2024, 2:03 AM
Unknown Object (File)
Dec 25 2023, 4:05 AM
Unknown Object (File)
Dec 3 2023, 9:15 PM

Details

Reviewers
epriestley
chad
Group Reviewers
Blessed Reviewers
Summary

Added phriction config option to specify the number of levels of hierarchy to display in the Document Hierarchy. Cleaning up my original pull request at https://github.com/facebook/phabricator/pull/338

Test Plan

After changing config option 'phriction.hierarchy-display-levels', document hierarchy will display that many levels.

Diff Detail

Branch
master
Lint
Lint Passed
Unit
No Test Coverage

Event Timeline

Did you test the "-1" case? I don't think it works for that cae looking at the code.

firejdl updated this revision to Unknown Object (????).Jan 24 2014, 10:57 PM

Fixed '-1' case.

Oops, you're right! I've now retested -1, 0, 1, 2, & 10. The code for -1 adds an extra almost identical query, so maybe it would be better to not include it & let people set the option to a sufficiently high number (999) instead? Too many levels will break the layout anyway.

I'd prefer a somewhat reasonable/conservative hard set of options (1-10 maybe?) Do you have specific need for more than 10? I am more concerned with people breaking layouts and filing bugs vs opening tasks to add more than 10 levels.

I don't really want to add config for this. There's almost no way it will survive, e.g., some of @chad's ideas around a CMS iteration on Phriction without needing to be reverted. This patch makes document queries more complex, which makes it harder to implement T4029. I think the removal of the // Fill in any missing children. block breaks the case where /x/ exists and /x/y/z/ exists, but /x/y/ does not exist.

chad requested changes to this revision.Apr 6 2014, 4:25 AM
chad added a reviewer: chad.

Pushing this out of my queue, sounds like we're not going in this direction at this time. Might be worth filing a Task though stating the core issue / reason this is useful and we'll see if there is more interest.

This revision now requires changes to proceed.Apr 6 2014, 4:25 AM