Problem/Motivation

It looks like there's quite a bit of refactoring of logic/code still planned for the Settings Tray module.

Before BigPipe was marked stable, we marked all classes & interfaces @internal, because BigPipe provides only functionality, not APIs.

I think that you probably want to do something similar for Settings Tray? I think you want to limit the API surface in a very clear, very simple way:

Any block that provides an off_canvas form in their block plugin annotation, is automatically picked up.

(Plus probably something for making things other than blocks configurable via the Settings Tray.)

Proposed resolution

  1. Mark as much as possible as @internal. That would include all PHP code, all HTML (Twig templates), all JS, all CSS. The only thing that's an API, is the declaration of "off_canvas forms".
  2. Add a outside_in.api.php file to document the actual API.

(And yes, off_canvas should be renamed to settings_tray, but that's probably part of #2803375: Rename Outside-in module to "Settings Tray" for real.)

Remaining tasks

TBD

User interface changes

None.

API changes

None.

Data model changes

None.

Comments

Wim Leers created an issue. See original summary.

Wim Leers’s picture

Wim Leers’s picture

This would mean that big tasks like #2784935: Outside In: Use Backbone for client-side state management can happen even after the module is marked stable.

tedbow’s picture

Status: Active » Needs review
FileSize
10.07 KB

Here is a patch.

tedbow’s picture

The only thing that's an API, is the declaration of "off_canvas forms".

and

Add a outside_in.api.php file to document the actual API.

The way we are marking form as off_cavnvas is in outside_in_block_alter but this is because these are existing blocks.
For a module that define it's own blocks it would be more correct to update add an 'off_canvas' form handler in their blocks plugin file.

How would that be documented in an .api.php file?

drpal’s picture

Status: Needs review » Needs work
+++ b/core/modules/outside_in/js/off-canvas.es6.js
@@ -5,6 +5,8 @@
+ * @internal

JSDoc does not use @internal. I'd suggest using @private or @protected depending on the actual goal of this documentation.

drpal’s picture

drpal’s picture

Status: Needs work » Needs review
xjm’s picture

Not RTBC-blocking, but we should definitely have a framework manager review this one.

+++ b/core/modules/outside_in/css/outside_in.tabledrag.css
@@ -3,6 +3,9 @@
  * @see tabledrag.js
+ *
+ *
+ * @internal

+++ b/core/modules/outside_in/css/outside_in.theme.css
@@ -1,6 +1,9 @@
  * Visual styling for Settings Tray module.
+ *
+ *
+ * @internal

Are there supposed to be double blank lines here?

xjm’s picture

+++ b/core/modules/outside_in/src/OutsideInManager.php
@@ -8,6 +8,8 @@
 class OutsideInManager implements OutsideInManagerInterface {

+++ b/core/modules/outside_in/src/OutsideInManagerInterface.php
@@ -4,6 +4,8 @@
 interface OutsideInManagerInterface {

If it's purely internal code, then why is there an interface for it?

xjm’s picture

@xjm is apparently a broken record. See #2835604-14: BigPipe provides functionality, not an API: mark all classes & interfaces @internal and @Wim Leers' reply in #15 following.

xjm’s picture

Wim Leers’s picture

Status: Needs review » Needs work

#5

How would that be documented in an .api.php file?

See core/core.api.php for plenty of examples of how to describe things in words, and not just using hook_modulename_blah().

#10++ — could not agree more.

Wim Leers’s picture

xjm’s picture

Issue tags: +Needs followup

Can we remove that interface, or spin off a separate issue to do so? Marking an interface @internal just seems like conflicting information, or "letter but not the spirit", outside of some weird/edgecase scenarios. Ideally we'd do both this and that before marking Settings Tray beta and I guess it wouldn't need to block this (we did it as a followup for BigPipe) but it does cause mental discordance for me in this patch. :P

Wim Leers’s picture

Issue tags: -Needs followup

#15: #2844261: Allow dialog links to specify a renderer in addition to a dialog type removed that interface altogether, so it's already taken care of :)

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

AFAICT this is ready. I have nits:

  1. +++ b/core/modules/outside_in/css/outside_in.details.css
    @@ -1,8 +1,9 @@
    -
    
    +++ b/core/modules/outside_in/css/outside_in.form.css
    @@ -1,8 +1,9 @@
    -
    

    I think these deletions are accidental.

  2. +++ b/core/modules/outside_in/css/outside_in.tabledrag.css
    @@ -3,6 +3,9 @@
      * @see tabledrag.js
    + *
    + *
    + * @internal
      */
    
    +++ b/core/modules/outside_in/css/outside_in.theme.css
    @@ -1,6 +1,9 @@
      * Visual styling for Settings Tray module.
    + *
    + *
    + * @internal
    

    @xjm already asked about this in #9.

… but they're not commit-blocking IMHO. Especially not for @internal things.