Page MenuHomePhorge

Update diffusion browse to generate Table of Contents when viewed as remarkup
Needs RevisionPublic

Authored by Sten on Nov 5 2023, 15:57.
Tags
None
Referenced Files
F399270: After.png
Nov 15 2023, 16:29
F399268: Before.png
Nov 15 2023, 16:29
F398751: Margins highlited.png
Nov 15 2023, 07:34
F390704: IndexOpen.png
Nov 6 2023, 21:47
F390698: IndexClosed.png
Nov 6 2023, 21:47
F390360: Example README.png
Nov 6 2023, 15:22
Tokens
"Love" token, awarded by valerio.bozzolan.

Details

Summary

Update the code such that when viewing a document in diffusion as remarkup, a table of contents (TOC) will automatically be generated, provided that the document has at least 2 headers!

Also fix t_block_interpreterests__/remarkup/interpreter-test.txt which was failing prior to this change (arc unit src/infrastructure/markup/remarkup/tests__/PhutilRemarkupEngineTestCase.php) - the block pattern match has been tightened up since this test case such that phutil_fake_test_block_interpreter is longer even recognised!

Fixes T15660

Test Plan

Diffusion:

Diviner:

  • Update a diviner document
  • Run bin/diviner generate
  • Confirm the diviner document TOC is unaffected

Differential:

  • Create a diff with headers in the description. Confirm all works as before with no TOC.
  • Create a diff comment with headers. Confirm all works as before with no TOC.

Diff Detail

Repository
rP Phorge
Branch
arcpatch-D25457
Lint
Lint Warnings
SeverityLocationCodeMessage
Warningwebroot/rsrc/js/application/diffusion/collapsible-container.js:1JAVELIN5`javelinsymbols` Not In Path
Unit
Tests Passed
Build Status
Buildable 936
Build 936: arc lint + arc unit

Event Timeline

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

Thanks again for this change

I like the general idea but I don't know how to improve it graphically. I mean, this is the current rendering for a simple README of two sections:

Example README.png (370×672 px, 28 KB)

As you can see, a visitor may be inclined to think that the README is a bit messed up and that have a strange alien list at the beginning.

I absolutely don't know how to improve this graphically, but we should do something more to separate our new table of content from the file itself, so that the TOC is just "some part of the page" and not part of the content.

src/infrastructure/markup/PhabricatorMarkupOneOff.php
122

Sorry my friend, I mean this, with just an array, as the documentation indicates under "Render a tag containing other tags safely"

Replace method_exists with instance of, as per review

Implement collapsing index

Sten marked an inline comment as done.Nov 6 2023, 21:47

The Javascript is inline for now, and we can look at putting it somewhere better if this diff is worth pursuing. Suggestions of where to put it welcome!

Here's our friend https://we.phorge.it/source/phorge/browse/master/src/docs/user/installation_guide.diviner?as=remarkup with Index closed:

IndexClosed.png (612×1 px, 89 KB)

And with index open:

IndexOpen.png (1×1 px, 144 KB)

src/infrastructure/markup/PhabricatorMarkupOneOff.php
122

Return an array when the function is expected to return a string-like object? I don't think that would be a good idea!
phutil_implode_html() is an excellent function though as it provides a guarantee that what it generates is safe HTML. Much better - no need for a maintainer to look any deeper for unsafe HTML.

I like the general idea but I don't know how to improve it graphically. I mean, this is the current rendering for a simple README of two sections:

I don't think any document with a header to text line ratio of 2:3 is going to look good without customised formatting.

src/infrastructure/markup/PhabricatorMarkupOneOff.php
122

(This thread is a non-blocking suggestion)

Premising that, to me, your solution is nice and readable and I see the point.

We also should see that in this function we already always return a phutil_tag(), where its third argument accepts an array for exactly this case where we have multiple HTML things to be appended. We have not a super-big documentation, but that is included in there, so I think it would be OK to take advantage of that.

Again, let's see the official example:

https://we.phorge.it/book/contrib/article/rendering_html/
phutil_tag('div', array(), array($header, $body));

So the suggestion was just about that.

Incidentally, doing this, we avoid to send a newline. In 100,000 years, this could save a network cable.

End of the tip ❤

This collapsible is interesting thanks!

Blocking tips:

  1. To reduce JavaScript, probably we can just have a .collapsible to have that closed, and a .collapsible.collapsible-active to show that. So, we can do most of the things from CSS. And JavaScript just toggles the .collapsible-active class.
  2. I don't know how, but we can create a separated JavaScript file to do this toggling thing, so this can be re-used later.

Non-blocking stuff:

  1. Maybe we should also evaluate this?
    Arrow indicating a proposal UX change in a Phorge README.png (168×1 px, 14 KB)

What do you think about? Maybe I can try doing something on 1. and 2. - I'm not opinionated on 3.

src/infrastructure/markup/PhabricatorMarkupOneOff.php
122

Actually I like the newline - makes debugging a little clearer. For low volume traffic like this, maintainability trumps performance.

Also, the 'div' tag adds a minimum of 11 chars, and additional unwarranted complexity.

Move javascript into a Javelin event.
Correct mistake in Javelin documentation.

Skipping lint because I cannot get javelinsymbols from https://github.com/phacility/javelin/tree/master to compile under OSX
Actually, it's the out-of-support 13 year old libfbjs (from https://github.com/facebookarchive/libfbjs) from it which won't compile.
I recommend javelin linting is dropped, and replaced with a more modern javascript linter.

Non-blocking stuff:

  1. Maybe we should also evaluate this?
    Arrow indicating a proposal UX change in a Phorge README.png (168×1 px, 14 KB)

I would really really like option 3, and would like to convert the index header to be a button in the button bar, but it's a bit beyond me at the moment.

Update the button such that it toggles between 'Open Index' and 'Close Index'

Remove "'hidden' => true," line from javelin config - no longer required.

  1. for CSS classes, it seems Phorge would use .collapsible-content instead of camel case
src/infrastructure/markup/PhabricatorMarkupOneOff.php
144

I very love this thing, thanks

  1. I just suggest container_id since the id can be set to whatever element in theory, not just divs.
webroot/rsrc/css/application/diffusion/diffusion-repository.css
27–29

At the moment .active is unused by us, so should be untouched. Let's un-declare.

webroot/rsrc/js/application/diffusion/remarkup-index.js
16 ↗(On Diff #1454)

Lovely! Feedback:

  1. Maybe container_id instead of div_id since we can apply this to any element in theory
  2. Maybe we can do something like this (untested):
var containerNode = JX.$(config.container_id);
JX.DOM.alterClass(containerNode, 'collapsible-content-active', !hidden);
  1. Maybe above "Open Index" we can add a similar TODO:
// TODO: Add i18n support.
// https://we.phorge.it/T15168
  1. Then we can declare the CSS class that activates the thing, so web designers can expand this to add astonishing animations (?)
.collapsible-content.collapsible-content-active {
  display: block;
}
  • Javascript updated to make collapsible containers more generic, and not specific to indices
  • Removed unused 'active' as per review
  • Moved setting the display of the container from Javascript to CSS as per review
  • Added i18n support for the button text (no need for a TODO!)
  • Extended variable/attribute names to make the code a bit more self-documenting
Sten marked 2 inline comments as done.Nov 12 2023, 11:05
NOTE: In short, feel free to just see the last diff and ignore this comment. If you like, nice! <3 If you don't, say so and I rollback. Thanks :D

I love your idea of using the config for internationalization and all these things.

Blocking thing:

  • we have some security benefits avoiding .innerHTML and using .innerText

Non-blocking ideas:

  • we can maybe unify the business logic avoiding to write twice collapsible-content-open and write twice alterClass etc.
  • if possible we can define var things at the beginning on the method and then to the rest (since these things are internally evaluated this way by JavaScript and by other languages, if I'm not wrong)
  • we have some advantages when migrating from closed = true to open = false
  • we can maybe use a sub-array like config.i18n.msg so to have less things

Since it would be totally boring for you to evaluate these things, and since Phorge allows to collaborate, and since I love the [edit] button in Wikipedia; I will "be bold" and propose a minor diff to your kind attention.

Feel free to see this amend and say: "Yeah, why not" or "AAAAAAAH please rollback NOW" and I will immediately rollback now. Thanks for your precious feedback :) and sorry if my edit was not appreciated.

Hoping to be useful, proposing minor things. Feel free to say "AAAH NOPE" :D

https://we.phorge.it/D25457?vs=1484&id=1502#toc

Happy to take your suggestions with a couple of changes

  • Remove index from the config labels - we've made collapsible-container.js generic, and not just for an index.
  • The reason for the if/else block was to enable long variable/attribute names so as to have self-documenting code. I've done away with it, but we do need a comment instead so as to show why we associate the 'push this to close' text with the open state and vice-versa.

Good to see it's not just me who can't run the javelin linter :-)

src/infrastructure/markup/PhabricatorMarkupOneOff.php
146–147

Non-blocking tip:

I'm quite sure that "butt" in English means culo in Italian. Maybe just "open" and "close". Or "btn_open" and "btn_close".

Renamed butt to btn as per PR. Much more common abbrieviation!

Tried to add a lovely icon but still keeping the label toggling and the whole button selectable

We need more help on this. Never approved anything similar, I don't have enough experience.

Probably somebody may want to generate random IDs so to support multiple elements (?)

By the way I've proposed a small icon. I tried to also avoid IDs with spaces.

We also may want to somehow neutralize the margin-top:0 that was set somewhere. I was not able to do that.

src/infrastructure/markup/PhabricatorMarkupOneOff.php
122

It's quite unique to have HTML id="" with spaces. Maybe with dashes. Proposed something

I can also propose another small change to adopt a sigil for the button label, so we don't need an ID.

Then we still need more help in review from others.

  • sorry if I introduced an ID for just the label: now replace with a non-unique Javelin Sigil
  • tried to fix the margin-top thing

As already noted we were affected by this margin-top: 0; that was making the TOC a bit too much attached to the button:

Margins highlited.png (152×366 px, 6 KB)

This is the rule affecting us, since we are the first in the page:

./webroot/rsrc/css/core/remarkup.css
body .phabricator-standard-page div.phabricator-remarkup *:first-child,
body .phabricator-standard-page div.phabricator-remarkup .remarkup-header + * {
  margin-top: 0;
}

I tried to workaround that and it seems to work.

But, we still need more help from experienced reviewers

webroot/rsrc/css/application/diffusion/diffusion-repository.css
50

Somebody may ask to move these .collapsible rules to a dedicated file. We may agree. But where exactly?

webroot/rsrc/js/application/diffusion/collapsible-container.js
2

I love that this is so generic! Thanks!

Maybe we can "BE BOLD" and move this under this path (?)

webroot/rsrc/js/core/behavior-collapsible.js

And rename diffusion-collapsible-container to just collapsible (?)

  • Renamed button/div IDs to remove spaces
  • Renamed diffusion-collapsible-container to collapsiblejx
  • Added margin-top: 10px; in the collapsible content - this overrides the higher up margin-top: 0px
  • Moved webroot/rsrc/js/application/diffusion/collapsible-container.js to webroot/rsrc/js/core/behavior-collapsiblejx.js
  • Renamed the JX from diffusion-collapsible-container to collapsiblejx - the single word collapsible was a bit too common if you 'git grep' for it.
  • move duplicated button text into a variable
  • improve formatting
src/infrastructure/markup/PhabricatorMarkupOneOff.php
125

(This is an old Revision that contains an example of addSigil() that is probably better than an ID on the label itself)

(Part 1)

webroot/rsrc/js/application/diffusion/collapsible-container.js
14–15

(Sigil thing - part 2)

src/infrastructure/markup/PhabricatorMarkupOneOff.php
125

I mean here there is the example with an icon () and the use of addSigil()

https://we.phorge.it/D25457?id=1516

I can also re-integrate if you like that icon

The problem there is that by requiring collapsiblejx to have knowledge of the 'collapsible-toggler-label', we introduce tight coupling to it's caller. (see how adding 'jx' on to the end of collapsiblejx has made it uniquely identifiable?)
A better solution would be to have the collapsiblejx config contain 'btn_open_icon' and 'btn_close_icon' icons. I'll have a go...

  • Refactored the PHP code to build the collapsible button & div into buildCollapsibleIndex()
  • Define the javelin call config first, then refer to it - saves defining the variables independently!
  • Create an icon field in the button, allowing for open and close icons to be set
  • Make both the icon and the text optional in the collapsible javascript - if you just want an icon, fine. Just text, also fine. Neither - sure!
Sten marked 2 inline comments as done.Nov 15 2023, 15:25

I am quite happy with this now. Spent way too long on it, so I hope you like it too!

JavaScript: ✅ I damn like your proposal and I think it is very OK, non-plus-ultra

CSS: 🔶 We can probably remove that margin-top: 10px;, and introduce this:

/* The "first element" in a Remarkup page receives a "margin-top: 0".
   This is normally OK, but ugly for the TOC, so we restore it.
   That "first element" selector is very specific, so, it's aggressive,
   so, we need important.
   The "margin-bottom" is copied since the inherited value may change. */
.collapsible-content > ul.remarkup-list {
  margin-top: 12px !important;
  margin-bottom: 12px;
}

So:

BeforeAfter
Before.png (235×504 px, 12 KB)
After.png (235×504 px, 11 KB)
NOTE: Nobody will see that. But After, I removed the white space between the TOC content and the button, and the TOC has top margin again.

PHP: (WARNING: I'M AN IMPOSTOR - I DON'T KNOW PHORGE WELL) Maybe somebody will propose to just use celerity_generate_unique_node_id() instead of our logic. Or, somebody may like to avoid new variables like $instance to just be shortcut for $this->collapsibleIndexInstance so to just use "toc-index-button-icon-{$this->collapsibleIndexInstance}" or sprintf("toc-index-button-icon-", $this->collapsibleIndexInstance) but avoiding $instance. But yeah we can probably just adopt celerity_generate_unique_node_id() and also drop collapsibleIndexInstance. Whatever we do, we probably do it wrong for somebody else. So just flip a coin.

src/infrastructure/markup/PhabricatorMarkupOneOff.php
166

Just a cute space here

Great suggestions

  • Implemented celerity_generate_unique_node_id()
  • Implemented .collapsible-content > ul.remarkup-list CSS

Loved the comment, as otherwise I wouldn't have understood what was going on in the CSS. I've reworded it - hope that's OK.

Skip lint (javelinsymbols)

Sorry if I've found two additional interesting things:

  1. we need to test against Phriction wiki. There, there is the Open Index button, doing nothing. Probably, there, it should not generate anything new/special.
  2. we probably need to skip TOC generation if the TOC body would be empty (tested against a Markdown file with just "asd asd" and no headings
src/infrastructure/markup/PhabricatorMarkupOneOff.php
166

Just a cute space here

This revision now requires changes to proceed.Nov 20 2023, 09:24

When a phriction markup page is generated , its synchronous, when a diffusion markup page is generated it's asynchronous.
This is going to take a bit of thought.