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:

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.

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:

And with index open:

In D25457#13043, @valerio.bozzolan wrote:

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.

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:

3. Maybe we should also evaluate this?
   

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

In D25457#13055, @valerio.bozzolan wrote:

Non-blocking stuff:

3. Maybe we should also evaluate this?
   

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.

5. for CSS classes, it seems Phorge would use .collapsible-content instead of camel case

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.

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.

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.

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

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

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

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

I don't know. Let's see :)

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:

Before	After

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.

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.