Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
Split off per #2782891-66: The Page Title block's title behaves in a confusing way with Settings Tray and the Help block incorrectly has Settings Tray styling.1.
Proposed resolution
Remaining tasks
User interface changes
API changes
Data model changes
Comment | File | Size | Author |
---|---|---|---|
#42 | 2897272-42.patch | 5.82 KB | Ada Hernandez |
#42 | interdiff-36-42.txt | 925 bytes | Ada Hernandez |
#26 | 2897272-26.patch | 5.9 KB | tedbow |
#21 | 2897272-21.patch | 5.12 KB | tedbow |
#21 | interdiff-17-21.txt | 977 bytes | tedbow |
Comments
Comment #2
Wim LeersHere's a start.
Comment #3
Wim LeersI 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.Comment #5
xjm#2782891: The Page Title block's title behaves in a confusing way with Settings Tray and the Help block incorrectly has Settings Tray styling is in now, so unpostponing.
Comment #6
tedbowThis 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.
Again doesn't mention related configuration.
Comment #7
Wim Leers#6
Comment #8
tedbowChanging to new settings_tray.module component. @drpal thanks for script help! :)
Comment #9
tedbowNeeds reroll at least for changes in #2803375: Rename Outside-in module to "Settings Tray" for real
Comment #10
jofitz CreditAttribution: jofitz at ComputerMinds commentedRe-rolled.
Comment #12
jofitz CreditAttribution: jofitz at ComputerMinds commentedRemoved out-of-context change.
Comment #13
tedbow@Jo Fitzgerald thanks for the re-roll!
Should we add something about surfacing related configuration here or in the help text? Because it does more that just editing block configuration.
Comment #14
Wim LeersThe 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.Comment #15
tedbow@Wim Leers ok sounds good!
Comment #16
xjmGreat 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.
Replace with:
And it can be joined with the next paragraph.
Replace with:
Replace with:
Replace "allows to change" with "allows changing".
Each bullet should be capitalized and end with a period.
This paragraph doesn't really belong in this section. It could be added to the next section instead.
Replace with:
(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.)
"Enable edit mode" is a bit confusing, "Edit" should be capitalized, and the sentence is a bit run-on. I'd say:
I wouldn't start a new paragraph here; if we do, it's unclear what "this" refers to. I'd instead say:
Comment #17
tedbow@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
Comment #19
tedbowTest failed b/c of "CI Job missing" 🤔 Retesting
Comment #20
xjmYeah maybe just remove it, so long as the API section has an @see to offcanvas?
Comment #21
tedbowok done
Comment #22
tim.plunkettLooks great!
Comment #24
tedbowretesting
Comment #25
Wim Leers"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?
Comment #26
tedbowI removed off-canvas from here:
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.settings_tray.api.php
I found a couple other uses ofoff-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.Comment #27
Wim LeersImportant fix! 👍
Comment #29
Wim LeersRetesting, was random infra fail:
Exception: Warning: apcu_store(): Unable to allocate memory for pool.
.Comment #31
Anonymous (not verified) CreditAttribution: Anonymous commentedRevert status after random fail.
Comment #32
xjm"an settings_tray form"
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.
Comment #33
xjmAccidentally deleted one bullet of my review so I'll have another comment shortly.
Comment #34
xjmSo, 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:
Comment #35
xjmOh 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."
Comment #36
tedbow@xjm thanks for the review
#32
#33 So tragic
#34
Fixed
#35
Fixed
Comment #37
xjmNah 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.
Comment #38
xjmAh 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.
Comment #39
xjmComment #40
xjmI can't read. What's already in the patch is better.
OK RTBC!
Comment #41
webchickA couple of nitpicks that stood out:
...to "be" configured
providing "a" settings_tray form.
Then, more of a philosophical question:
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)
Comment #42
Ada Hernandez CreditAttribution: Ada Hernandez at MTech, LLC commentedonly nitpicks
Comment #43
xjmSo 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. :)
Comment #44
Wim LeersI 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.
Comment #45
webchickOk, 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!