The current hovercard of a wiki document has no further information except the title. This commit adds object type, project tags, parent documents, last author and last edited time to the card.
Preview:
Preview in a pessimistic case:
Closes T15433
Details
Details
- Reviewers
valerio.bozzolan avivey Cigaryno - Group Reviewers
O1: Blessed Committers - Maniphest Tasks
- T15433: Hovercard of wiki article needs more information
- Commits
- rPdf3ac50cb742: Implements a more informative hovercard for wiki documents
Edit a wiki document with/without project tags and parent documents and see the hovercard in the feed.
Diff Detail
Diff Detail
- Repository
- rP Phorge
- Branch
- arcpatch-D25303
- Lint
Lint Passed - Unit
Tests Skipped - Build Status
Buildable 592 Build 592: arc lint + arc unit
Event Timeline
There are a very large number of changes, so older changes are hidden. Show Older Changes
Comment Actions
I like it:
But I wonder if we should list Tags in a dedicated line, just after the "Last Edited"
Somehow like in Maniphest, where Tags are at the bottom:
What do you think about?
Comment Actions
I wonder if other Hovercards have a quantitative limitation on things like Tags, to be used as additional inspiration
Comment Actions
I reckon: no.
But I wonder if we should list Tags in a dedicated line, just after the "Last Edited"
Somehow like in Maniphest, where Tags are at the bottom:
What do you think about?
I am more against this. First of all: I use the interface of the hovercard. The only thing I can add at the end is a new combination of Attribute: Description. If I would want to add only the tag list, I have to render every part of the hovercard by myself - overwriting the whole default template. Second: I think wiki documents are not like tasks. So even if you can add many tags, I would say that is a very unlikely use case.
Comment Actions
Feel free to replace the screenshot with something else ihih
But I'm still +1 - and thanks for the clarification
Comment Actions
I like this idea. A few things I want to note
- Could you post a screenie for what the breadcrumb rendering appears as?
- Does the name of this new class follow other classes that extend the hierarchy? I’m guessing yes but just want to double-check.
- Maniphest tasks will render their primary Space (assuming non-default) as a prefix to the title. I think wiki docs should do similar.
- Do wiki docs have a description field?
Comment Actions
- Yeah, all other hovercard extensions start with the application name too.
- That is the default behavior for every hovercard. So here too.
- Never seen something like this in the forms. I have looked into the tables: No column found.
| src/applications/phriction/engineextension/PhrictionHovercardEngineExtension.php | ||
|---|---|---|
| 83 | This label may not be necessary as it should be apparent what this is, but maybe style this slightly different? What do you think about putting this as the first content above the project tags? | |
| src/applications/phriction/engineextension/PhrictionHovercardEngineExtension.php | ||
|---|---|---|
| 83 | I know what you mean. But to put it before the tags means to put it before the object type "Wiki document", which my be counter intuitive... I see, what I can cook up about it. | |
| src/applications/phriction/engineextension/PhrictionHovercardEngineExtension.php | ||
|---|---|---|
| 83 | Stupid question: what happens if the field has a NULL key? | |
| src/applications/phriction/engineextension/PhrictionHovercardEngineExtension.php | ||
|---|---|---|
| 83 | ||
Comment Actions
If a third person leaves a positive comment I would be for approving it in the master, also to receive more feedback, or eventual follow-up graphical suggestions :)
Comment Actions
Moves parent document links to top (merging them with always visible link of wiki homepage) and project tags to extra paragraph.
Comment Actions
My new proposal:
Sorry for skipping unit tests, but I develop with our live env which of course has some extra commits changing some things for our firm. So I arc diff with just a checked out phorge repo without database access (hence no unitn testing)...
Comment Actions
Nice!
I don't have any idea about the homepage, but having no parents leave the icon alone in a row now, to me:
Comment Actions
Nice!
I don't have any idea about the homepage, but having no parents leave the icon alone in a row now, to me:
Strange. For me the Wiki homepage is rendered, because every document has at least this page as ancestor. How does it look with a parent in your system?
Comment Actions
This looks great! Thank you for working on it.
I’ve been working on a docker-compose setup to get a development environment up and running with as little friction as possible. If you’re interested check out https://github.com/neandrake/phab-dev
Comment Actions
Code-wise this looks good, though I’m not overly familiar with CSS or how Phorge uses it. Ready to accept after the empty icon thing is resolved.
| webroot/rsrc/css/phui/phui-hovercard.css | ||
|---|---|---|
| 5 | Accidental indentation? | |
| webroot/rsrc/css/phui/phui-hovercard.css | ||
|---|---|---|
| 5 | Yeah, seems that way. Will fix this with the missing wiki homepage. | |
Comment Actions
Fixed the indentation and @valerio.bozzolan should now see the breadcrumb. But even if all else fails, at least Wiki Document will be rendered...
Comment Actions
Showing project tags might be unique to tasks - Revisions don't show them, and neither do Owners or Commits. It might make sense not to show it here either.
It might make sense to move the Ancestor calculation to willRenderHovercards(), which is called once for multiple hovercards, and can make a single DB call for all of them (though I'm not sure of when that actually happens). The result of that call is passed to renderHovercard() in the $data parameter.
| src/applications/phriction/engineextension/PhrictionHovercardEngineExtension.php | ||
|---|---|---|
| 30–36 | Isn't there a shortcut method somewhere to get project phids on an object? I'd expect it to be used very often. | |
| 101 | Can't we use a SearchEngine here? See https://we.phorge.it/source/phorge/browse/master/src/applications/phriction/controller/PhrictionController.php$60 . | |
| webroot/rsrc/css/phui/phui-hovercard.css | ||
| 119 | I'm a little surprised that there's a need for a new CSS class? | |
Comment Actions
Formally "requesting changes", but only for the inline SQL code. Everything else I said is just "a suggestion".
Comment Actions
Very nice
| src/applications/phriction/engineextension/PhrictionHovercardEngineExtension.php | ||
|---|---|---|
| 101 | Good call, this should be updated to something similar to the linked | |
Comment Actions
I would like to make a case for tags. The hovercards, which do not show tags, are somewhat special and in their own context: revisions and commits belong to repositorys, users and projects have no project relations in the sense of other objects. But every other object is better contextualized with tags in my opinion.
| src/applications/phriction/engineextension/PhrictionHovercardEngineExtension.php | ||
|---|---|---|
| 30–36 | Well, this was my reference: https://we.phorge.it/source/phorge/browse/master/src/applications/project/engineextension/PhabricatorProjectsCurtainExtension.php$24 | |
| webroot/rsrc/css/phui/phui-hovercard.css | ||
| 119 | First of all: I have written it in a way to be reused for other hovercards. Second: yeah, for the the rendering of the ancestral breadcrumbs and the inline tags there were no styles which could be used out of the box. | |
Comment Actions
Use willRenderHovercards() to get all project and ancestor handles of potential multiple documents and rewrites inline SQL query.
Comment Actions
I hope, this is now ready to land... sorry for the long delay 😅
| src/applications/phriction/engineextension/PhrictionHovercardEngineExtension.php | ||
|---|---|---|
| 101 | I have rewritten the querying of the ancestors and I have refactored all data querying to get done in willRenderHovercards(). | |
| src/applications/phriction/engineextension/PhrictionHovercardEngineExtension.php | ||
|---|---|---|
| 62 | ||
Comment Actions
Premising that I liked this before, and I like this even more now,
Probably, the tags, if present, should be listed as last entry. I suggest this since it's an info of variable-length, that is probably better in the footer.
But, last word to avivey since proposed to omit Tags at all. Honestly I like them. Why not.
| src/applications/phriction/engineextension/PhrictionHovercardEngineExtension.php | ||
|---|---|---|
| 43 | ||
Comment Actions
Would be, as already said, a big rewrite, because then I can not use the default template, but have to render every aspect of the card myself. I would prefer an ellipsis like utilized in the dashboard list views. But even so: I don't think it is realistic that a wiki article has as many tags as a task, even if possible. So it doesn't seem like a feasible use case.
Comment Actions
Yeah no problem. Still thank you. From me it's still a soft +1, just few tips about if (count($array)) → if ($array).
Comment Actions
Small issue in the query, but otherwise looks good.
| src/applications/phriction/engineextension/PhrictionHovercardEngineExtension.php | ||
|---|---|---|
| 99 | This makes a separate DB call for each $document - see https://we.phorge.it/source/phorge/browse/master/src/infrastructure/edges/query/PhabricatorEdgeQuery.php$128 I think the "full query" is something like $edges = id(new PhabricatorEdgeQuery())
->withSourcePHIDs(mpull($documents, 'getPHID'))
->withEdgeTypes(array($edge_type))
->execute();(In reality, I don't think there's a way to actually get here with multiple docs, but still... :/) | |
| webroot/rsrc/css/phui/phui-hovercard.css | ||
| 119 | 👍 | |
Comment Actions
Okay, now how do I land this? Just arc land? Again, in my current setup there is no way of unit testing, so I don't know if it is possible...
| src/applications/phriction/engineextension/PhrictionHovercardEngineExtension.php | ||
|---|---|---|
| 99 | I have refactored this into one query. | |
Comment Actions
Yeah just try "arc land" and, if it does not work, we can land this for you
But please mention any error message in case :) so T15096 ...