Page MenuHomePhorge

Implements a more informative hovercard for wiki documents
ClosedPublic

Authored by bekay on Jun 17 2023, 17:28.
Tags
None
Referenced Files
Unknown Object (File)
Sat, May 25, 07:52
Unknown Object (File)
Sat, May 25, 06:11
Unknown Object (File)
Sat, May 25, 04:29
Unknown Object (File)
Sat, May 25, 02:51
Unknown Object (File)
Fri, May 24, 16:04
Unknown Object (File)
Wed, May 22, 22:27
Unknown Object (File)
Mon, May 20, 21:42
Unknown Object (File)
Sun, May 19, 04:13

Details

Summary

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:

image.png (154×402 px, 7 KB)

Preview in a pessimistic case:

Preview pessimistic page.png (219×402 px, 28 KB)

Closes T15433

Test Plan

Edit a wiki document with/without project tags and parent documents and see the hovercard in the feed.

Diff Detail

Repository
rP Phorge
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

I like it:

D25303 2023-06-19 proposal.png (211×402 px, 28 KB)

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:

Task Hovercard.png (61×400 px, 5 KB)

What do you think about?

I wonder if other Hovercards have a quantitative limitation on things like Tags, to be used as additional inspiration

I wonder if other Hovercards have a quantitative limitation on things like Tags, to be used as additional inspiration

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:

Task Hovercard.png (61×400 px, 5 KB)

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.

valerio.bozzolan mentioned this in Z1: Phorge.

Feel free to replace the screenshot with something else ihih

But I'm still +1 - and thanks for the clarification

Run tests again (since they are marked as skipped now)

I like this idea. A few things I want to note

  1. Could you post a screenie for what the breadcrumb rendering appears as?
  2. Does the name of this new class follow other classes that extend the hierarchy? I’m guessing yes but just want to double-check.
  3. Maniphest tasks will render their primary Space (assuming non-default) as a prefix to the title. I think wiki docs should do similar.
  4. Do wiki docs have a description field?
In D25303#8876, @speck wrote:

I like this idea. A few things I want to note

  1. Could you post a screenie for what the breadcrumb rendering appears as?
  2. Does the name of this new class follow other classes that extend the hierarchy? I’m guessing yes but just want to double-check.
  3. Maniphest tasks will render their primary Space (assuming non-default) as a prefix to the title. I think wiki docs should do similar.
  4. Do wiki docs have a description field?
  1. image.png (159×411 px, 11 KB)
  2. Yeah, all other hovercard extensions start with the application name too.
  3. That is the default behavior for every hovercard. So here too.
  4. 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

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 :)

Moves parent document links to top (merging them with always visible link of wiki homepage) and project tags to extra paragraph.

My new proposal:

image.png (154×402 px, 7 KB)

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

Nice!

I don't have any idea about the homepage, but having no parents leave the icon alone in a row now, to me:

Home test.png (239×402 px, 28 KB)

Nice!

I don't have any idea about the homepage, but having no parents leave the icon alone in a row now, to me:

Home test.png (239×402 px, 28 KB)

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?

This looks great! Thank you for working on it.

In D25303#8980, @bekay wrote:

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

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

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.

Fixes missing ancestors and css indentation.

bekay marked an inline comment as done.Jun 24 2023, 11:25

Fixed the indentation and @valerio.bozzolan should now see the breadcrumb. But even if all else fails, at least Wiki Document will be rendered...

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
webroot/rsrc/css/phui/phui-hovercard.css
119

I'm a little surprised that there's a need for a new CSS class?

avivey requested changes to this revision.Jun 24 2023, 13:35

Formally "requesting changes", but only for the inline SQL code. Everything else I said is just "a suggestion".

This revision now requires changes to proceed.Jun 24 2023, 13:35

Very nice

src/applications/phriction/engineextension/PhrictionHovercardEngineExtension.php
101

Good call, this should be updated to something similar to the linked

In D25303#9015, @avivey wrote:

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.

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
It is literally responsible for all tag rendering in views with curtains (that is, the menu on the right side, if I am not mistaken).

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.

Use willRenderHovercards() to get all project and ancestor handles of potential multiple documents and rewrites inline SQL query.

bekay marked 2 inline comments as done.Jul 30 2023, 10:42

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

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
bekay marked an inline comment as done.Jul 31 2023, 07:47

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.

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.

In D25303#10646, @bekay wrote:

Probably, the tags, if present, should be listed as last entry.

Would be, as already said, a big rewrite ...

Yeah no problem. Still thank you. From me it's still a soft +1, just few tips about if (count($array))if ($array).

bekay marked 3 inline comments as done.Jul 31 2023, 09:13

Complete some issues.

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

👍

This revision is now accepted and ready to land.Jul 31 2023, 09:26
bekay marked an inline comment as done.

Optimizes querying of project edges of multiple documents.

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.

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