Ah, thanks for landing

This collapsible is interesting thanks!

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.

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!

Replace method_exists with instance of, as per review

Thanks again for this change

In D25420#12963, @speck wrote:

Instead of adding a checkbox is there precedent for having a separate button alongside Cancel and Submit?

Replacing 'new PhutilSafeHTML' with 'phutil_implode_html' as per review. Much nicer!

Change PHID list to have new lines instead of commas

In D25362#12819, @valerio.bozzolan wrote:

Hi @Matthew can I help in landing this useful change?

Looks OK from XSS perspective - protection already in place. Ready for review!

Need to put in some XSS protection.

Restore fixed interpreter-test.txt

This looks like a reasonable chance to me.

Nice

Great points. Thank you for talking through more details.

Nice, thank you!

Would anyone be willing to give this another review? TIA

Rephrase comment

In D25450#12945, @speck wrote:

Maybe add some doc - the view and edit actions being lumped together are because it would be a larger change to split out that functionality right now, correct?

Show 404 error instead of policy based message

Instead of adding a checkbox is there precedent for having a separate button alongside Cancel and Submit?

I’m surprised the monograms weren’t already defined on the applications - those should be somewhere already, right?

Basically it works

😂

This looks good and I really like the idea of being able to customize the style of external links. Just one tweak to the logic I think we should add before landing.

Looks good to me, I suggest clarifying the comment before landing.

Maybe add some doc - the view and edit actions being lumped together are because it would be a larger change to split out that functionality right now, correct?

Hoping to be useful, I will land this after 2023-11-07 :) Thanks for this nice feature

Hoping to be useful, I will land this in 3 days :) since I love this feature

Basically it works

Ah! Indeed it would be lovely

An example of the TOC in action is in the diviner pages.

Where I can see the TOC in action? I think that I do not see that even here without ?as=remarkup:

However, it would make sense not to query transactions in applications which once were installed and are now uninstalled and not accessible anymore anyway.
In downstream https://phabricator.wikimedia.org/T344232 , the exception is Query (of class "ConpherenceTransactionQuery") overheated.
However we uninstalled Conpherence more than four years ago in https://phabricator.wikimedia.org/T127640 (but of course the DB data is still around).

Create a dedicated method getRemarkupLinkClass() to write that once

fix test

Integrate some tips. Clarify that this is just a compromise between performance and a good default, only to be used for target="_blank" purposes, to don't slow down Remarkup.

Hoping to be useful, implement tips, to be easily tested.

I am sorry, thanks for your last comment, I now understand your point. Yes, I guess we'd better return new Aphront404Response() instead of some permission based message. Like we'd do trying to access the URL of an uninstalled application. Now just need to find out how to do that :)

Thanks for clarifying this behavior, it sounds like contact numbers in general need fleshed out quite a bit.

In D25452#12901, @speck wrote:

I’m guessing the user would be unable to remove their contact number (or even see it), unless turning off sms deletes their contact number. In this case I think the user would still want to know it’s saved somewhere and could remove it.

Changing policy based on sms being configured seems a little off to me. Having the setting only conditionally show based on it being configured seems fine, however what happens in this scenario:

1. Turn on sms
2. Add number
3. Turn off sms

you just adopted PHP 7.4 or something similar I think to fix

Thanks for reporting

Nevermind, this is a problem with my setup, sorry for the noise. (I missed the second line, and of course I realized it right after submitting)