Comments

Wim Leers created an issue. See original summary.

Wim Leers’s picture

Issue tags: +Usability, +DX (Developer Experience)
FileSize
1.74 KB

Here's a start.

Wim Leers’s picture

Title: [PP-1] Fix module description, also describe scope in *.api.php file » [PP-1] Fix module description, hook_help(), and document module scope in *.api.php file
FileSize
2.57 KB
4.27 KB

I realized that hook_help() should also be updated. Since Settings Tray is so closely related to the Quick Edit module (it also provides in-place editing, just of something different, and it also builds atop the Contextual Links module), I mirrored the help after that module's help.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

xjm’s picture

Title: [PP-1] Fix module description, hook_help(), and document module scope in *.api.php file » Fix module description, hook_help(), and document module scope in *.api.php file
Status: Postponed » Active
tedbow’s picture

Status: Active » Needs work
  1. +++ b/core/modules/outside_in/outside_in.api.php
    @@ -10,6 +10,19 @@
    + * The goal of the Settings Tray module is to make administering blocks easier,
    + * and therefore to make the site building experience faster and more pleasant.
    

    This doesn't doesn't mention anything about expose configuration that is related to blocks but is not the block configuration itself, for instance menu edit form or site title.

  2. +++ b/core/modules/outside_in/outside_in.info.yml
    @@ -1,6 +1,6 @@
    -description: 'Provides the ability to change the most common configuration from the Drupal front-end.'
    +description: 'In-place editing of block configuration.'
    

    Again doesn't mention related configuration.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
1.47 KB
4.8 KB

#6

  1. Fixed!
  2. IMHO for non-developer end users that's exactly what this says: configuration of blocks. They don't know the technical details. Module descriptions need to err on the side of simplicity, not accuracy.
tedbow’s picture

Component: outside_in.module » settings_tray.module

Changing to new settings_tray.module component. @drpal thanks for script help! :)

tedbow’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll
Jo Fitzgerald’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
4.96 KB

Re-rolled.

Status: Needs review » Needs work

The last submitted patch, 10: 2897272-10.patch, failed testing. View results

Jo Fitzgerald’s picture

Status: Needs work » Needs review
FileSize
1.51 KB
4.96 KB

Removed out-of-context change.

tedbow’s picture

@Jo Fitzgerald thanks for the re-roll!

+++ b/core/modules/settings_tray/settings_tray.info.yml
@@ -1,6 +1,6 @@
-description: 'Provides the ability to change the most common configuration from the Drupal front-end.'
+description: 'In-place editing of block configuration.'

Should we add something about surfacing related configuration here or in the help text? Because it does more that just editing block configuration.

Wim Leers’s picture

Should we add something about surfacing related configuration here or in the help text? Because it does more that just editing block configuration.

The point is to KISS.

Look at the description for media. It also doesn't convey everything. That's not the point. The point is that it's understandable.

tedbow’s picture

Status: Needs review » Reviewed & tested by the community

@Wim Leers ok sounds good!

xjm’s picture

Status: Reviewed & tested by the community » Needs work

Great to see these docs being added! I think they have all the right information.

We can remove some words from this to make it more digestible. xjm uncaps her red copy-editing pen.

  1. +++ b/core/modules/settings_tray/settings_tray.api.php
    @@ -10,6 +10,29 @@
    + * The goal of the Settings Tray module is to make administering blocks easier,
    + * and therefore to make the site building experience faster and more pleasant.
    

    Replace with:

    The Settings Tray module makes administering blocks easier.

    And it can be joined with the next paragraph.

  2. +++ b/core/modules/settings_tray/settings_tray.api.php
    @@ -10,6 +10,29 @@
    + * It achieves this by building on the infrastructure that the Contextual
    + * Links module provides: Settings Tray provides a "Quick Edit" contextual link
    

    Replace with:

    It provides a "Quick Edit" link for blocks using the Contextual Links
    module.

  3. +++ b/core/modules/settings_tray/settings_tray.api.php
    @@ -10,6 +10,29 @@
    + * for blocks. Clicking this contextual link opens the Settings Tray, which
    

    Replace with:

    Clicking this link opens the off-canvas Settings Tray.

  4. +++ b/core/modules/settings_tray/settings_tray.api.php
    @@ -10,6 +10,29 @@
    + * allows to change the rendered contents of the block by showing all relevant
    

    Replace "allows to change" with "allows changing".

  5. +++ b/core/modules/settings_tray/settings_tray.api.php
    @@ -10,6 +10,29 @@
    + * - for every block, one can configure whether to display the block title or
    + *   not, and optionally override it (block configuration)
    + * - for menu blocks, one can configure which menu levels to display (block
    + *   configuration) but also the menu itself (menu configuration)
    + * - for the site branding block, one can change which branding elements to
    + *   display (block configuration), but also the site name and slogan (simple
    + *   configuration)
    

    Each bullet should be capitalized and end with a period.

  6. +++ b/core/modules/settings_tray/settings_tray.api.php
    @@ -10,6 +10,29 @@
    + * The Settings Tray uses a variant of a standard dialog: an off-canvas dialog.
    

    This paragraph doesn't really belong in this section. It could be added to the next section instead.

  7. +++ b/core/modules/settings_tray/settings_tray.info.yml
    @@ -1,6 +1,6 @@
    -description: 'Provides the ability to change the most common configuration from the Drupal front-end.'
    +description: 'In-place editing of block configuration.'
    

    Replace with:

    Provides a sidebar to configure blocks on the page.

    (The module descriptions are supposed to start with a verb. In the past couple years there was a big initiative to make them all follow this pattern.)

  8. +++ b/core/modules/settings_tray/settings_tray.module
    @@ -18,10 +19,14 @@ function settings_tray_help($route_name, RouteMatchInterface $route_match) {
    +      $output .= '<p>' . t('To edit blocks in place, you can either enable \'edit mode\' in the toolbar and click the block, or choosing Quick edit from the contextual links of a block (see the <a href=":contextual">Contextual Links module help</a> for more information about how to use contextual links).', [':contextual' => \Drupal::url('help.page', ['name' => 'contextual'])]) . '</p>';
    

    "Enable edit mode" is a bit confusing, "Edit" should be capitalized, and the sentence is a bit run-on. I'd say:

    To edit blocks in place,
    either click the Edit button in the toolbar and then click on the block,
    or choose Quick edit from the block's contextual link. (See the Contextual Links module help for more information about how to use contextual links.

  9. +++ b/core/modules/settings_tray/settings_tray.module
    @@ -18,10 +19,14 @@ function settings_tray_help($route_name, RouteMatchInterface $route_match) {
    +      $output .= '<p>' . t('This will open the Settings Tray for this block, where a compact form will be displayed, with configuration that controls what the block shows. After saving, the page reloads, and the effects are visible.') . '</p>';
    

    I wouldn't start a new paragraph here; if we do, it's unclear what "this" refers to. I'd instead say:

    The Settings Tray for the block will open in a sidebar, with a compact form for configuring what the block shows. Save the form and the changes will be immediately visible on the page.

tedbow’s picture

Status: Needs work » Needs review
FileSize
4.56 KB
4.84 KB

@xjm thanks for the review
1. fixed
2. fixed
3. fixed
4. fixed
5. fixed
6. The next section is about the API provided for this module. The off-canvas dialog is not part of the API. I am wondering if we need this at all.
7. fixed
8. fixed
9. fixed

Status: Needs review » Needs work

The last submitted patch, 17: 2897272-17.patch, failed testing. View results

tedbow’s picture

Status: Needs work » Needs review

Test failed b/c of "CI Job missing" 🤔 Retesting

xjm’s picture

6. The next section is about the API provided for this module. The off-canvas dialog is not part of the API. I am wondering if we need this at all.

Yeah maybe just remove it, so long as the API section has an @see to offcanvas?

tedbow’s picture

ok done

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

Looks great!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 21: 2897272-21.patch, failed testing. View results

tedbow’s picture

Status: Needs work » Reviewed & tested by the community

Build timed out (after 110 minutes). Marking the build as aborted.

retesting

Wim Leers’s picture

+++ b/core/modules/settings_tray/settings_tray.api.php
@@ -10,9 +10,31 @@
+ * link opens the off-canvas Settings Tray which allows changing the rendered
...
+ * which uses the off-canvas dialog.

+++ b/core/modules/settings_tray/settings_tray.info.yml
@@ -1,6 +1,6 @@
+description: 'Provides a sidebar to configure blocks on the page.'

"sidebar" vs "off-canvas" vs "dialog" vs "Tray"

As a core developer, I understand the nuances, but I think this is pretty confusing for most?

tedbow’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
5.9 KB
1.8 KB
  1. @Wim Leers I think you are right in #25.

    I removed off-canvas from here:

    link opens the off-canvas Settings Tray which allows changing the

    Then the only instance of "off-canvas" in the this file is followed by a @see to the off-canvas.es6.js file.

    I think using "sidebar" is ok in settings_tray.info.yml because this will be seen in the modules page by site builders and we can't assume they will look at the code at so I think "sidebar" is probably the most common and recognisable word for this.

  2. In updating the settings_tray.api.php I found a couple other uses of off-canvas that were out of date.

    We were referring to setting the "off-canvas form" in the annotation but since #2904134: Settings Tray uses the off-canvas dialog type, but "off_canvas" is not an accurate form plugin name, "settings_tray" is settings_tray is used in the annotation.

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community
+++ b/core/modules/settings_tray/settings_tray.api.php
@@ -55,16 +54,16 @@
- * off-canvas form:
+ * settings_tray form:

Important fix! 👍

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 26: 2897272-26.patch, failed testing. View results

Wim Leers’s picture

Status: Needs work » Reviewed & tested by the community

Retesting, was random infra fail: Exception: Warning: apcu_store(): Unable to allocate memory for pool..

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 26: 2897272-26.patch, failed testing. View results

vaplas’s picture

Status: Needs work » Reviewed & tested by the community

Revert status after random fail.

xjm’s picture

Status: Reviewed & tested by the community » Needs work
  1. I think the link is "Quick edit" (which is the correct casing) but in one place the docs have "Quick Edit". Let's correct the latter wherever it appears. ("Quick Edit" with title case should only be used when referring to "the Quick Edit module").
  2. +++ b/core/modules/settings_tray/settings_tray.api.php
    @@ -33,16 +54,16 @@
    + * Finally, blocks that do not specify an settings_tray form using the
    

    "an settings_tray form"

  3. +++ b/core/modules/settings_tray/settings_tray.module
    @@ -18,10 +19,15 @@ function settings_tray_help($route_name, RouteMatchInterface $route_match) {
    +      $output .= '<p>' . t('To edit blocks in place, either click the <strong>Edit</strong> button in the toolbar and then click on the block, or choose "Quick edit" from the block\'s contextual link. (see the <a href=":contextual">Contextual Links module help</a> for more information about how to use contextual links).', [':contextual' => \Drupal::url('help.page', ['name' => 'contextual'])]) . '</p>';
    

    The sentence beginning with "see" needs to be capitalized. Edit: Also, very minor nitpick, but the final period can go inside the paretheses since the entire sentence is part of the parenthetical.

xjm’s picture

Accidentally deleted one bullet of my review so I'll have another comment shortly.

xjm’s picture

+++ b/core/modules/settings_tray/settings_tray.api.php
@@ -10,9 +10,30 @@
+ * The Settings Tray module makes administering blocks easier. It provides a
+ * "Quick Edit" link for blocks using the Contextual Links module. Clicking this
+ * link opens the Settings Tray which allows changing the rendered contents of
+ * the block by showing all relevant configuration (minus block visibility
+ * conditions). This means blocks can be modified without leaving the page.

So, yeah, what I noticed is that this only mentions the "Quick edit" contextual link and not the "Edit" mode button and interactive block hovers. We could mention both; however, since this is API documentation and not user documentation, I don't think we actually need to explain the user interface. We just need to explain what the developer can do with it.

So I think we can just say:

The Settings Tray module allows blocks to configured in a sidebar form without leaving the page. For example:
- [Same existing list here]
-
-

Block visibility conditions are not included the sidebar form.

xjm’s picture

+++ b/core/modules/settings_tray/settings_tray.api.php
@@ -10,9 +10,30 @@
- * By default, every block will show its built-in form in the Settings Tray.
+ * By default, every block will show its built-in form in the Settings Tray
+ * which uses the off-canvas dialog.

Oh this paragraph is also weird. I'd say:
"By default, the Settings Tray shows any block's built-in form in the off-canvas dialog."

tedbow’s picture

Status: Needs work » Needs review
FileSize
3.85 KB
5.75 KB

@xjm thanks for the review

#32

  1. The link is "Quick edit". Fixed. There 2 case of "Quick Edit" in the .module file but not in the areas we are changing. Let me know if you want me to fix those in this patch also
  2. Fixed. I also fixed a line a couple down where the word "this" could be move to the above line.
  3. Fixed

#33 So tragic

#34
Fixed

#35
Fixed

xjm’s picture

The link is "Quick edit". Fixed. There 2 case of "Quick Edit" in the .module file but not in the areas we are changing. Let me know if you want me to fix those in this patch also

Nah it looks like those two are in inline comments, so out of scope here. Thanks!

Looks good; I'll leave for someone else to review though since I proposed the edits.

xjm’s picture

Ah one thing is missing from the patch: the improvement for the module description. (It is in the issue title.)

We can simplify the description a bit. My proposal is:
Allows changing the most common configuration from the Drupal front-end.

xjm’s picture

Status: Needs review » Needs work
xjm’s picture

Status: Needs work » Reviewed & tested by the community

I can't read. What's already in the patch is better.

OK RTBC!

webchick’s picture

Status: Reviewed & tested by the community » Needs work

A couple of nitpicks that stood out:

The Settings Tray module allows blocks to configured in a sidebar form

...to "be" configured

 providing an
- * off-canvas form:
+ * settings_tray form:

providing "a" settings_tray form.

Then, more of a philosophical question:

-description: 'Provides the ability to change the most common configuration from the Drupal front-end.'
+description: 'Provides a sidebar to configure blocks on the page.'

Here (and elsewhere), we're removing language about Settings Tray being a generic way to configure common properties, and making it about blocks specifically. While I realize that's what's currently been implemented in the "1.0 release", the end goal all along was for this to be a generic UI component that could be hooked into by a variety of core + contrib modules. (See https://dri.es/examples-of-how-to-make-drupal-outside-in)

Is there a particular reason we're stripping this back? At a glance, that seems to make it less appealing to contrib + other parts of core to integrate with it, which is kinda the opposite of what we're going for here? (And also unclear why it's called "Settings Tray" and not "Quick Block Config" or something—NOT that we should rename it again! :D)

Adita’s picture

Status: Needs work » Needs review
FileSize
925 bytes
5.82 KB

only nitpicks

xjm’s picture

Here (and elsewhere), we're removing language about Settings Tray being a generic way to configure common properties, and making it about blocks specifically. While I realize that's what's currently been implemented in the "1.0 release", the end goal all along was for this to be a generic UI component that could be hooked into by a variety of core + contrib modules. (See https://dri.es/examples-of-how-to-make-drupal-outside-in).

So I mentioned this to Angie briefly already but: from my perspective it's actually the offcanvas library that is the generic UI component (and we already have a great example of other "Drupal becoming outside in" with the Layout Builder, which uses the offcanvas library but does not depend on Settings Tray). Everything in Settings Tray really is about blocks: the UI interaction, the under-the-hood implementation, the API provided to contrib. It can be extended by core and contrib... if you (as a developer) want to hook into configuring blocks in an awesome user-friendly UI. If you (as a developer) don't want to hook into configuring blocks, you probably just want the offcanvas library. :)

My concern with the current description is that "Provides the ability to change the most common configuration from the Drupal front-end" is really not true at all (and also kind of vague and confusing). I'd say "the most common configuration" is content types, fields, views, etc... none of which can be configured with Settings Tray. Some day those things might be configurable in a different offcanvas implementation. If some day Settings Tray encapsulates several offcanvas implementations for different things, then we can update the module description, extend the API docs, etc. which would be the docs gate for that feature. :)

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

I mostly agree with @xjm: the Settings Tray module in its current incarnation is tightly coupled to blocks. That is why A) it depends on the Block module, B) its API only supports blocks, C) its functionality only supports blocks.

Generalizing it to work for things other than blocks would be great, but that's not the current reality. When we do generalize it, we can make the description more general too. But until then, it'd be misleading.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Ok, thanks for the clarification.

Conferred with @yoroy and he agreed with erring on the side of accuracy here.

Committed and pushed to 8.5.x. Thanks!

  • webchick committed 92787ef on 8.5.x
    Issue #2897272 by tedbow, Wim Leers, Jo Fitzgerald, Adita, xjm: Fix...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.