Problem/Motivation
Proposed resolution
Remaining tasks
User interface changes
API changes
Data model changes
Problem/Motivation
For modules to use the off-canvas dialog provided by the Settings Tray module without the Settings Tray
The Settings Tray module besides what its name suggests provides a lot more that just a tray. It provides a quick way to edit blocks and other configuration and also expands edit mode to disable links and form element. This might not be desired for every site and there should be an easy way to turn this off without losing the ability for other module to use the tray.
Proposed resolution
Move 'off_canvas' dialog renderer from the Settings Tray module to the core dialog system so that can be used in the same way as the regular dialog(provided by core but not used by core) and modal dialog.
Remaining tasks
Agree on this or a better to bring in consistent off-canvas tray experience to core and contrib that doesn't rely on Settings Tray module.
User interface changes
A new off-canvas dialog/tray could be use by core and contrib.
API Additions
No API changes are made. All existing dialog/modal links will remain same, but the following are API additions:
New off_canvas dialog renderer.
Used like:
$mylink['attributes'] = [
'class' => ['use-ajax'],
'data-dialog-type' => 'dialog',
'data-dialog-renderer' => 'off_canvas',
];
To understand dialog renderers generally @see #2844261: Allow dialog links to specify a renderer in addition to a dialog type
Data model changes
None
Comment | File | Size | Author |
---|---|---|---|
#171 | offcanvas_dog_small.gif | 177.27 KB | tedbow |
#163 | 2784443-163.patch | 56.47 KB | tedbow |
#163 | interdiff-159-163.txt | 1.78 KB | tedbow |
#159 | 2784443-159.patch | 56.35 KB | tedbow |
#159 | interdiff-155-159.txt | 11.3 KB | tedbow |
Comments
Comment #2
nod_Comment #3
tedbowComment #4
nod_Core dialogs provide this functionality and more. Just need a little glue code for positioning I don't think we need a new lib for this.
Comment #5
tedbow@nod_ I like the direction #2786459: "Offcanvas" tray should be using the existing dialog system is going but I still like the idea of this current issue. I think we need to make it as easy as possible to use the "offcanvas" style tray.
I don't that glue code that is being developed in #2786459: "Offcanvas" tray should be using the existing dialog system belongs in the outside_in.module when it is stable.
Core and contrib should be able to use this new tray just by setting
'data-dialog-type' => 'offcanvas',
and maybe attaching a library without outside_in enabled. That way contrib doesn't have to provide the glue code each time. We can then have a consistent DX and UX experience for a tray.Comment #6
nod_Like I was saying on IRC yesterday, I don't want a new data-dialog-type type, it's supposed to be following the HTML5 spec and there is only dialog or modal that make sense.
I do agree with the fact that the positioning code shouldn't be in the outsidein module in the end. I think we should extend the dialogs to add some sort of named positioning option. The current default behavior for modals is center-of-displaced-area, this new one would be a displacing-sidebar kinda thing.
Ideally we would add a
drupalPosition
option todata-dialog-options
(just like we added drupalAutoButtons) and name this onesidebar
or something.Comment #7
Wim Leers#2786459: "Offcanvas" tray should be using the existing dialog system happened. It's not clear what's next here, or if this can be closed. Per #65–#68 in that issue, a follow-up is still needed. Perhaps this can be that follow-up?
Comment #8
tedbow@Wim Leers updated title and summary to reflect change for #2786459: "Offcanvas" tray should be using the existing dialog system
Comment #9
tedbowOk. Here is the first try at the patch.
Remaining tasks
1. Move CSS
2. Figure out other remaining tasks ;)
Comment #10
tedbowWhoops forgot to attach patch!
Comment #12
tedbowRemoving
{% spaceless %}
from template because this causes tests to fail.Comment #14
tedbowOk this patch
Comment #16
droplet CreditAttribution: droplet commentedJust my little thoughts,
I think it's not stable enough to move to CORE. In experiment module section, it gets committed to Git faster than CORE.
Here're few problems I thought it could be addressed in outside_in module threads first and then sync it back to here:
- Almost all CSS selectors aren't following the CSS Naming conversion (SMACSS)
(Personally, I'm fine with the IDs. But I thought the Drupal banned. Needs a check on CSS standard page and frontend persons)
#offcanvas or .offcanvas should be the main component.
then
#offcanvas-X
#offcanvas-wrapper / #offcanvas__wrapper
#offcanvas__child
It should not double #id #id.
Off-topic:
I think in other experiment modules, we could remind the developers to prefix the name with "experiment" in between. Therefore, any movement, we just search & replace it instead.
It's good for Core & Contributed usages.
Needed to document what it is
this two lines seems unrelated to this scope
Comment #17
tedbow@droplet ok I think you are correct.
Hopefully the patch I made will at least point at needs to be done.
I have just gone through the outside_in.module issue queue and marked every issue that deals only with the offcanvas portion of the module as a child issue of this 1. There are currently 5(might find more). I will try to keep them up to date. Once all the child issues are fixed we can good ahead with this.
I have just update #2815831: Move Off-canvas related CSS from drupal.outside_in library to drupal.off_canvas with some of @droplet's suggestions about css selectors. Lets solve that in that issue.
Comment #18
tedbowUpdate: almost all the child issues have been committed.
#2820135: Allow any contextual link to opt-in to being displayed via offcanvas is RTBC. I just requeued a 8.2x test but hopefully this will be committed soon
I started #2844261: Allow dialog links to specify a renderer in addition to a dialog type for something to work on while the child issues to this were being finished. I think probably I will able to close that and bring that back in here.
#2843901: Settings tray should use toolbar models to close the toolbar items is a 1 line javascript fix that should be committed before this 1(or just incorporated here) but should not stop working on this issue.
Comment #19
tedbowOk this patch moves the the Offcanvas tray out of Setting module.
Some notes
Comment #21
tedbowOk, great all test passed except for those related to stable theme. I need to confirm but I think we need to copy any new library assets into the theme.
Also noticed this issue has not been fixed so marking as child of this #2828912: Offcanvas width is not reset if tray is open with different width and not closed
It is small change but should stop review of this issue.
Comment #22
tedbowOk added the new template and the css to the stable theme. That should allow the Stable*Override tests to pass.
Also remove the dependency on outside_in from a test module.
Comment #24
tedbowOk this patch does a number of things
I will comment after I update so I can use dreditor.
Comment #25
tedbowNotes on previous patch.
This the current so the @todo can be removed. Several more in this patch.
The CSS was indented 4 spaces instead of 2.
Moved the css added in #2793849: Handle offcanvas differently at lower widths from outside_in.css to offcanvas.css because this behavior should happen without outside_in enabled.
Left this portion of the @todo referencing this issue. Because I was not clear if we actually need to this.
Will the transitions cause problems for more complex layouts?
Should not need to enable outside_in for any system tests.
Update the tests to use the correct library. To get these links to work only drupal.ajax library is needed. \Drupal\Core\Render\MainContent\OffCanvasRender will handle attaching offcanvas library when link is used.
Just corrected the test group.
Comment #26
tedbowThere are inconsistencies in the files concerning whether "off canvas" should be considered 2 words.
In php classes the pattern is 2 words like OffCanvasRender.php.
But in css and js 1 word. offcanvas.css and offcanvas.js
I am not sure on the standard for css and js(when not a js class) file names. - vs _ for word splits. I see both in core. I see naming standards for inside js and css files but not the files themselves
If someone can point be to the standards that would be great.
But first look.
Name should change to off-canvas.css
change to: off_canvas.js
I think the standard for js files is underscores but not sure.
change to: off_canvas.motion.css
Comment #27
martin107 CreditAttribution: martin107 as a volunteer commentedThis link has the variable naming convention, and the case study is a useful read.
https://www.drupal.org/node/1887918#separate-concerns
As for the css filename convention here is some relevant text from a different document
https://www.drupal.org/node/1887922
Anyway I hope that helps... if you still have lingering question ... just let me know and I will dig around some more
Comment #28
tedbow@martin107 thanks for the links. This patch does a huge rename of offcanvas to off-canvas or off_canvas depending on the context.
Comment #29
tedbowUpdated summary to mention related issues.
Also crossed of tasks already done
Comment #30
tim.plunkettOnly nitpicks left, the actual code moves are 100% complete.
There are still 14 instances of "offcanvas" or "Offcanvas" in core and 13 of "off canvas".
96 of "off-canvas" and 46 of "OffCanvas" are fine.
Missing an extra *
Missing blank line
Note the CSS problems are also in the Stable theme version, need to be fixed twice.
Comment #31
GrandmaGlassesRopeManJust minor changes to
ajax.js
which appear reasonable to me.Comment #32
tedbow@tim.plunkett thanks for review!
1. I replace all of these. I also updated a bunch of comments to make referring to the off-canvas dialog consistent.
In various places in the comments it was referring to it as
So now it always refers as "off-canvas dialog" in the comments.
2,3 Fixed in css, also in stable.
@drpal Thanks for confirmation!
Comment #33
tim.plunkettThanks! I believe this is ready.
Comment #34
xjmThanks everyone!
Let's get a JavaScript subsystem maintainer review on this issue (if available) since it is making a noteworthy JS API addition to stable core.
Comment #35
tedbowRemoving #2843901: Settings tray should use toolbar models to close the toolbar items as related and in summary because this only happens in the Setttings Tray module outside_in.js which staying in the module. Not the off-canvas dialog
Comment #37
nod_It looks good, one question though.
What is the exact public API we're introducing that is bound to our major semver, if any? I'd like to have droplet opinion on this to know if the api we have is sufficient. He had more use of it than me.
Comment #38
tedbow@nod_ thanks for the review.
If updated the "API Changes" portion of the IS
Comment #39
tedbow@tim.plunkett pointed out that these are API additions not changes. All existing dialog links will remain the same, no changes needed to use the existing dialog and modal.
Updated the summary
Comment #40
webchickAssigning to droplet for review.
Comment #41
webchickAlso bumping priority, since this is the main blocker to getting this functionality stable.
Comment #42
droplet CreditAttribution: droplet commentedBusy life, can't find time to review big patch. But now I realized I only need to review a very small part in JS core. Setting Tray is still in Experimental module section.
Should we document it better? To tell it will be "drupal_dialog.off_canvas" and defined in PHP..etc.
(I guessed this is a class name for HTML wrapper at first glance, lol)
===
this condition but easier to read?
OffCanvasRenderER?
what is %100?
Comment #43
tedbow@droplet
Totally understand, thanks for the review.
1. I agreed this could use an expanded comment.
Updated the comment drupalDialogRenderer but first I had to update comment above it about [dialogType]. These 2 are used together. Personally it took a while to figure when I was first looking at the dialog system how dialogType affected selection of renderer php service.
So I updated these 2 comments, even though dialogType wasn't introduced in this issue the description really needed to be expanded because it is necessary to explain how drupalDialogRenderer works.
2. I am not really sure what you are suggesting here. Could you explain?
3. Nice catch updated.
4. Expanded the comment to explain further.
Comment #44
droplet CreditAttribution: droplet commentedmy original thought but now I think we can skip this:
I got the meaning by reading source code but I still don't understand what is %100 in above comment. Is it equal to 100%?. (Not a native English speaking :) As developer, I first thought that is: minDisplaceWidth % 100 = 68 )
OK. So to my understanding, minDisplaceWidth is the breakpoint to trigger $mainCanvasWrapper padding. It need not to match CSS all time.
Is it redundant?
namespaced with drupal if it's not jQuery UI attr.
data-drupal-offset
Code started from above LINE:
This is what we coded in other scripts in CORE. Personally, I don't like this code style and I wonder if we want to make the same mistake again. It forced to patching by swapping the whole off-canvas.js. Or dirty coding, adding another listener.
For example,
If we want to keep the padding above. We adding another listener.
1. This is set to ZERO
2. This is set back to padding value.
In a complicated system, it will lose in control.
EDITED:
even
minDisplaceWidth
can't override.Comment #45
droplet CreditAttribution: droplet commentedIt's good to provide an option for this. Maybe, for some theme, placed at LEFT is better than RIGHT. Not just for RTL.
Comment #46
tkoleary CreditAttribution: tkoleary at Acquia commentedWell, no. Not unless we want to write the logic that will automatically move the *toolbar* vertical tray to the right when settings tray is to the left. Otherwise we could have two trays, toolbar and settings tray, on one side which would be very odd.
The questions we need to be asking are not 'might some theme or module developer want to do this?' but 'what user problem would this solve?'. In this case none that I can see.
Comment #47
SKAUGHT#46
i recall some of the early comments about outside_in about the possibility of the tray being on other sides (ie: top & bottom). which as a generic tool, the tray 'would' be possible to be anywhere for other functionality beside related block settings..
ie: a content overview tray at the bottom for Roles who are Editors.
@trays_everywhere. (:
Comment #48
tedbow@droplet thanks for your review again in #44
Sorry it took me so long to get back to it I was looking at other related issue.
I am wondering we should postpone this until these issues are resolved:
#2826722: Add a 'fence' around settings tray with aggressive CSS reset.
#2847522: Allow off-canvas links to be rendered by either "default" or "admin" renderer
#2853208: [META] Determine best method ensure consistent theming of Off Canvas Tray
We could commit this 1 first but probably will less confusing to get those fixes in first.
But let me address you review so we have a record.
Sorry for this it is not your English but my typing :(
It should say "100%"
#44.1 Yes this appears to be redundant. I have commented it out locally and I can't see a change in behaviour. I tried resizing the dialog and resizing the window and it all seems to happen smoothly.
#44.3 I am not really sure about the point on the coding style
Re:
Are you talking about this
$mainCanvasWrapper.css('padding-' + edge, 0);
I am not sure what is so complicated about this.
When the dialog is closed the padding that was added so that dialog didn't cover up the body needs to be removed.
If not the padding will remain and there will be a empty space.
Maybe I am missing something.
Comment #49
tkoleary CreditAttribution: tkoleary at Acquia commented@tedbow
I think so. I would say the order should be to commit:
And then this issue.
Comment #50
droplet CreditAttribution: droplet commentedThanks.
and an issue to rename offcanvas to off_canvas? It reduces a huge amount of changes and better patch to review.
Comment #51
tedbow@droplet
Yes that is good idea.
@MaxFire is working on this #2862625: Rename offcanvas to two words in code and comments. at DrupalDevDays in Seville. It is very close.
Postponing on that issue and #2826722: Add a 'fence' around settings tray with aggressive CSS reset.
Comment #52
tedbowComment #53
miro_dietiker@tedbow Why do we need to postpone this issue on the fence one?
It seems to me it's mostly only a CSS thing and doesn't change anything like an API?
Comment #54
tedbow@miro_dietiker is delayed on #2826722: Add a 'fence' around settings tray with aggressive CSS reset. because:
@DyanneNova was working the CSS reset I will check with her to see how close she is to finishing it.
Comment #55
tedbowComment #56
tedbowOk #2826722: Add a 'fence' around settings tray with aggressive CSS reset. is mostly staying at RTBC so I think it is time to start on this.
Since the last patch in #43 2 important issues have landed
This should make this patch much cleaner.
I will CSS changes from #2826722: Add a 'fence' around settings tray with aggressive CSS reset. in this patch for now. I will also include a "no-css" patch without the changes. This patch will not work but should be easier to review. The no CSS patch is 60k smaller.
Once #2826722 lands we should just have to apply this patch and move off-canvas.*css files from outside_in/css to core/misc/dialog/css.
@todo Include changes to need to /core/themes
@see #43 to get an idea of what that entails
Comment #57
xjmSince this will be adding a new API to stable core, we should probably also draft a change record that briefly explains what the library is and how to use it.
Comment #58
xjmComment #59
tedbowHere is the draft change record: https://www.drupal.org/node/2896596
Comment #61
Wim LeersThis can't be right?
Ok so this is moved from Settings Tray's JS.
So all CSS is moved, except the one for the toolbar. That sounds sensible.
However, shouldn't this depend on the new
drupal.dialog.off_canvas
library then?Why are we moving this into core? I already questioned the need for this at #2894358-3: Front-end Review of Settings Tray module for stabilization. I thought this was only necessary for the Settings Tray JS functionality?
Moving this from Settings Tray into "core core" means that we at the very least need very clear documentation about why this is necessary.
Oh, nicely done, looks like this was nicely planned long before this issue could happen :)
This comment now needs to be updated.
Comment #62
Wim LeersComment #63
tedbow@Wim Leers thanks for the review!
1. Copied comment to #2826722-120: Add a 'fence' around settings tray with aggressive CSS reset.
2. Yes, since we decide not to do #2847522: Allow off-canvas links to be rendered by either "default" or "admin" renderer we can always apply this class. Change made in #2826722: Add a 'fence' around settings tray with aggressive CSS reset.
3. Yes you are correct it should depend on
drupal.dialog.off_canvas
After #2826722: Add a 'fence' around settings tray with aggressive CSS reset. is committed we can update that
4. It is required see #2896960: Updated inline docs for data-off-canvas-main-canvas in twig template
5. Yep you could always use the tray if the user didn't have access to Settings Tray other functionality so we needed test for it by itself.
6. Updated
Comment #64
Wim LeersRegarding #61.1/#63.1/#2826722-120: Add a 'fence' around settings tray with aggressive CSS reset.:
Which made me realize I forgot something in my review above:
We still have this an
outside_in_css_alter()
— do we really need to? Because that means maintaining this hack in multiple places.Comment #65
tedbowCopied CSS to stable theme and updated stable.info with library override
Comment #67
Wim LeersSince this is blocked by #2826722, I posted #2826722-126: Add a 'fence' around settings tray with aggressive CSS reset. to explain that that is indeed a blocker, and bumped it to major.
We can keep rolling patches here, but at least updating the title to make it clear that this needs another patch to land first.
Comment #68
tedbowOk hopefully this will fix 2 failing tests.
The CSS need to be moved up 1 directory so that in stable there wasn't /css/.../css/off-canvas*.css
No other CSS in core/misc have a sub "css" folder anyways so that should be removed.
Needed to copy the template file to stable also.
Comment #70
tedbowThis patch takes different approach. It does not include any changes #2826722: Add a 'fence' around settings tray with aggressive CSS reset.
So it adds the Off-canvas dialog from the Settings Tray module but does not do a CSS reset.
So if you open the dialog with the links provide by the test module or any other use of the off-canvas dialog except Settings Tray. To test this you can enable the test module off_canvas_test and visit /off-canvas-test-links
If you use the Settings Tray "Quick Edit" block forms they will have current CSS which makes them dark themed.
This is much simpler patch to review hopefully.
Comment #71
tedbowwhoops uploaded the file twice 😰
Comment #72
tim.plunkettShame this is still needed. Would have been nice just to change \Drupal\Core\Render\Element\Page directly, but it can't depend on things in system.module. Hmm...
Would this make more sense to live in \Drupal\Tests\Core\Ajax, as it isn't testing a system.module thing?
This isn't the first time I've wanted a Drupal\Core subsystem to be able to provide a template :(
Comment #73
tedbow@tim.plunkett thanks for review.
1. Yes assuming because 3 below we can't change this
2. Yes moved
3. Not sure if that is possible. Not done I guess
Comment #74
GrandmaGlassesRopeManJust a move of the
off-canvas
JavaScript. 🎉Comment #76
tim.plunkettThis code is RTBC in my opinion. Removing the subsystem review tag from #34 after the review in #74.
This still needs a CR and an IS update now that it is not blocked on the fence CSS issue.
Comment #77
xjmIt'd also be good for the IS update to include the pros and cons of doing this before the fence issue, so the frontend framework managers can take that into consideration. Thanks!
Comment #78
tedbowUpdate the summary
Removed old parts dealing with #2844261: Allow dialog links to specify a renderer in addition to a dialog type
which was split into its own issue committed.
Looking to see if we actually need to bring in some CSS dealing with animation of the main canvas resize.
Comment #79
tedbowSo this patch adds the off-canvas.motion.css which provides the CSS animation for resizing the main canvas when the off-canvas dialog is open or resize.
The version of the off-canvas.motion.css file used in this patch is from the latest clean patch #2826722-112: Add a 'fence' around settings tray with aggressive CSS reset.
This file is currently in the outside_in module but it is not actually loaded in the library.
@see #2856441: Off-canvas CSS files not included in libraries file but we marked that as duplicate where solving in #2826722-92: Add a 'fence' around settings tray with aggressive CSS reset.
So the this now only contains CSS necessary to animate the resizing of the main-canvas smoothly. All the of the other CSS in the reset has been left out of this patch.
The off-canvas dialog in this patch is fully functional but will just take on the styles of the currently active theme.
Comment #81
tedbow#79 was strange test failure. Retesting.
But also since I just added the CSS animation wait it could be that the animation timing is messing up the test.
So I added wait() that is slightly longer than the animation.
This fixed it locally for me.
Not sure why we are not seeing this failure on #2826722: Add a 'fence' around settings tray with aggressive CSS reset.
Comment #83
Wim LeersWhy is it no longer blocked on the fence CSS issue? My understanding is that this doesn't make sense without the fence CSS issue, because any module using the API/capabilities that this patch introduces would have to do its own CSS overrides?
Comment #84
tedbowWe don't actually need to attach the
core/drupal.dialog.off_canvas
library we can just attach thecore/drupal.dialog.ajax
library and only attach it once.\Drupal\Core\Render\MainContent\OffCanvasRenderer will handle attaching
core/drupal.dialog.off_canvas
. I think this better example for the test because the developer making a link should not have to worry about which library they need to attach for each dialog renderer. Just use the library for ajax libraries and the correct other libraries will be attached in the response.Re #83
I think it should still wait on the https://www.drupal.org/node/2826722 but I think this patch can be rerolled when it is finished.
Right it is much simpler to understand what is going on without those changes.
I do think that this patch could be committed on its own. If we found a way to mitigate this problem because this patch itself does give a fully functional dialog in the same way as our current dialogs which also don't have a CSS reset. But haven't time to think more on that.
Comment #86
tedbowOk there is really weird testing stuff going on here 😱
#81 first did not fail
I retested #81 failed because of unrelated NodeTypeTranslationTest failure
#84 failed because of OutsideInBlockFormTest::testBlocks twice
which is weird because the only change between #81 and #84 is a change to \Drupal\off_canvas_test\Controller\TestController::linksDisplay() (I doubled check this)
which should have absolutely no effect on OutsideInBlockFormTest::testBlocks()
Why on Why 😢
Comment #87
Wim LeersAt least a dozen RTBC issues failed in the past 24 hours, due to two commits that have since been reverted. This was likely also a victim!
Comment #88
tedbowSetting to needs review for last changes
Comment #89
tedbowremoving #2828912: Offcanvas width is not reset if tray is open with different width and not closed
#2820135: Allow any contextual link to opt-in to being displayed via offcanvas
from IS because fixed and outdated
Fixed issue summary removing tag
Removing "Needs change record" tag. @see https://www.drupal.org/node/2828912
Comment #90
tim.plunkettRestoring the other tags.
Why are these files so different? Sorry if that was discussed already
It's strange to see these additions without corresponding removals. @tedbow tells me they were added via JS before, that part should be removed.
Comment #91
tedbow#90.1 This is because I took the file from this issue #2826722: Add a 'fence' around settings tray with aggressive CSS reset.
We should wait until that issue committed until we bring that file in so removing it and references to it and the copy in stable theme.
#90.2 Yes this class was added in off-canvas.es6.js but no longer needs to be. Fixed
Comment #92
tim.plunkettThat makes sense, thanks.
This looks great!
Comment #93
tedbowRe-rolling after https://www.drupal.org/node/2826722
This patch also moves all the CSS that deals with the off-canvas dialog for Settings Tray module into /core/misc/dialog
Then adds these overrides to stable theme.
2 other things
Removes modules not necessary for OffCanvasTest from the test $modules var
Added the animation wait for off-canvas dialgo to \Drupal\Tests\system\FunctionalJavascript\OffCanvasTestBase::waitForOffCanvasToOpen
Comment #94
tedbowOk a few clean ups for testing.
Comment #95
tedbowA couple fixes from #93 reroll.
stable.info
reference to the wrong folder for the css files.Comment #98
tim.plunkettLooks great
Comment #99
star-szrMinor: Templates in themes (not module-provided) shouldn't have @ingroup themeable per https://www.drupal.org/node/1354#templates.
Super minor, pre-existing condition (it's like this in the current template), shouldn't hold up commit. But there's an extra bit of whitespace at the end of this tag. Can be fixed in a tiny followup issue, maybe with similar things.
This is looking good, I can't do a very in-depth review at the moment but there have already been a lot of good reviews :)
Comment #100
tedbow@Cottser thanks for taking a look! Fixed both issues in #99
Comment #101
star-szrThanks @tedbow!
The @ingroup themeable should stay in this template, but be removed from the stable template.
For a bit more context, the themeable group lists all the templates provided and should only have the "originals": https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Render%21....
In HEAD (and 8.3.x) there are templates not following this convention but yeah :)
Comment #103
tedbow@Cottser yep sorry, that was my intention. Most have bee confused about which file I was editing.
Fixed
UPDATED: Setting back to RTBC because it was set to needs work because I aborted the tests in #100 to remove it from the testing queue since we have a new patch.
Comment #105
star-szr👍
Comment #106
Wim LeersStill missing the docs for this group thing. Otherwise we make core less maintainable.
Does this move mean these classes should no longer be
@internal
?Nice!
Why can we remove these?
Why do we need to add this?
These are styles not for Settings Tray, but for the Off-canvas dialog.
"off canvas tray" -> "off-canvas dialog"
s/See/@see/
Not entirely sure though.
This is that issue. That means this todo should be removed.
s/tray/dialog/
More of this.
Again a todo that's being solved in this very patch.
More still.
The library name is not updated.
s/tray/dialog/
Comment #107
tedbow@Wim Leers thanks for the thorough review as always!
1. Added comment to explain
2. Remove @internal for OpenOffCanvasDialogCommand. This is the only 1 that other could should be calling directly. If returning an AjaxResponse then a module might want to add a OpenOffCanvasDialogCommand command to open the off-canvas on return.
3. Yes!
4, At some point for ajax dialog links to work with data-dialog-renderer we needed outside_in.js in this library that was actually changed in #2844261: Allow dialog links to specify a renderer in addition to a dialog type so right now not positive if it was needed after that issue.
5. Now for link to work with
You don't actually have to attach
core/drupal.dialog.off_canvas
. You only need to attachcore/drupal.dialog.ajax
. This allows the genericdata-dialog-renderer
logic to work. ThenOffCanvasRenderer
will be selected as the content render service.OffCanvasRenderer
will then attachcore/drupal.dialog.off_canvas
.So developers only have to know about setting
'data-dialog-renderer' => 'off_canvas',
They don't have to know which core library supports this renderer, since now no extra module needs to be enabled.6. Updated comment
7. fixed
8. fixed
9. I true comment has never been stated. Fixed
10. Replaced all with "off-canvas dialog"
12. Removed these todos, made sure the string "2784443" is nowhere in core.
13. replaced
14. updated library name. Made sure the string "outside_in/drupal.off_canvas" is nowhere in core
16. fixed
Comment #108
Wim Leers#107.4: okay, so #2844261: Allow dialog links to specify a renderer in addition to a dialog type should in theory have already done this. No problem then.
#107.5: makes sense. Especially because
outside_in/drupal.off_canvas
(nowcore/drupal.dialog.off_canvas
) already depends on it. Thanks for the explanation!#107.12: 👍 for diligence
#107.14: 👍 for diligence
I wanted to RTBC, but grepping for "tray" still yields several results:
.
Comment #109
Wim LeersAddressed my own remarks in #108 (Ted is on vacation).
Comment #110
Wim LeersI figured I'd do some manual testing to ensure it all still works correctly.
I quickly noticed that some icons were missing:
Root cause:
This is in
core/misc/dialog/off-canvas.base.css
. The URL resolves to/misc/icons/ffffff/pencil.svg
, not/core/misc/icons/ffffff/pencil.svg
.CSS files in this patch are:
core/modules/outside_in/css/*.css
tocore/themes/stable/css/core/dialog/*.css
core/misc/dialog/*.css
In case 1, the depth relative to the root changes from 5 to 6.
In case 2, the depth relative to the root changes from 5 to 4.
This means that in both cases, we need to change the
url(…)
statements, but that didn't happen for either one. Working to fix that…Comment #111
droplet CreditAttribution: droplet commentedI'm not against the changes but can we make a quick patch in outside_in module first?
for example these changes. I expect this patch as simple as move around the file names only.
Less or more, I think we have diff and more loose code standard in experimental modules. for example:
We preferred const modalHeight in this case.
jQuery UI
and many comments changes..etc
and I'd say this issue is a blocker: #2830882: Introduce Drupal.offCanvas, mirrored after Drupal.dialog, to avoid Settings Tray using Drupal.Dialog in unintended ways
Don't want to see a new technical debt soon :)
Move into CORE, I think we need more general code comment. eg.:
standardized off-canvas selection color.
I think we need to doc why it's not same as CORE. Or it's a bug..
Comment #112
Wim Leers9 lines
url(…)
statements changed incore/misc/dialog
, 9 changed incore/themes/stable/css/core/dialog
.I think this is now RTBC'able again.
Comment #113
Wim LeersThe name & comments made sense until this issue. They make sense to change here. (But yes, it'd have been even better if that had already happened.)
These are both pretty extreme nitpicks which can easily be follow-ups.
Hm, having read that issue, I'm inclined to agree. OTOH, that could also quite easily be a follow-up, because it'd only be refactoring internals. I agree though that significant changes like this should happen before adding new stable functionality to core…
Comment #114
droplet CreditAttribution: droplet commentedComment #115
tedbow@Wim Leers and @droplet thanks for pushing forward!
The patch in #109 should have also update the CSS in core/themes/stable/css/core/dialog
Same with #112 but the paths to the icons need to be slightly different since they should references icons in core/themes/stable/images/core/icons
Fixing both these in this patch.
Comment #116
star-szrWill need a reroll after #2830882: Introduce Drupal.offCanvas, mirrored after Drupal.dialog, to avoid Settings Tray using Drupal.Dialog in unintended ways and probably #2899724: Fixes for settings tray CSS and JavaScript minor issues but there are a couple things that I noticed that can be touched up in the meantime.
We need to not add this back please because #2897320: Settings Tray causes the "pointer" cursor to be used both when hovering over inaccessible links (should use "default" cursor) and when in input[type=text] (should use "text" cursor) :)
Minor: Adds one blank line too many.
Comment #117
star-szrAlso, this patch could be generated with the diff.renames = copies option to shrink the patch and make it easier to review.
Comment #118
tedbow#116.1 fixed
2 fixed
#117 I added the line to my .gitconfig as described in the linked doc but it did not change the size of the file which I had thought it should.
Comment #119
star-szrI diffed the patches, the changes look good, needs a reroll because I committed #2830882: Introduce Drupal.offCanvas, mirrored after Drupal.dialog, to avoid Settings Tray using Drupal.Dialog in unintended ways, might need another one after #2899724: Fixes for settings tray CSS and JavaScript minor issues so whatever works :)
Attaching an example with diff.renames = copies, doesn't apply to HEAD and marking do not test.
Comment #120
tedbowHere is reroll of #118.
Everything in applied except for changes in off-canvas.*.js files.
I manually moved those files and then added the changes from #118 to off-canvas.es6.js and then rebuilt off-canvas.js
Sorry I am still not getting the diff.renames = copies documentation to work for me.
I have
In my ~/.gitconfig file
I run
git config -l
and I see "diff.renames=copies"
I am running git diff like
git diff -M25%
I don't want to derail this issue with git help but if anyone sees what I am doing wrong feel free to ping me on irc or drupal slack.
Comment #122
tedbow#120 does not apply because #2830882: Introduce Drupal.offCanvas, mirrored after Drupal.dialog, to avoid Settings Tray using Drupal.Dialog in unintended ways was reverted.
Not sure it makes sense to reroll until unless that issue is going to take long. I just set it back to RTBC
Comment #123
tedbowSetting back to needs review. Patch should apply now that https://www.drupal.org/node/2830882#comment-12221025 was fixed
Comment #125
tedbowI was wrong patch didn't apply here reroll
Comment #126
star-szr#2899724: Fixes for settings tray CSS and JavaScript minor issues is in now, so hopefully just one more reroll (verified #125 doesn't apply). Thanks @tedbow!
Comment #127
tedbow@Cottser here is the reroll @tim.plunkett finally help me the get the
2784443
thing working alsoComment #129
star-szrI'm ignoring the revert on #2830882: Introduce Drupal.offCanvas, mirrored after Drupal.dialog, to avoid Settings Tray using Drupal.Dialog in unintended ways for this review :)
On a general note, the off-canvas.es6.js file refactored in #2830882: Introduce Drupal.offCanvas, mirrored after Drupal.dialog, to avoid Settings Tray using Drupal.Dialog in unintended ways needs some docs cleanup to meet core gates.
Blank line should be added above @type line. Not going to call out every instance in the file.
Blank line should be added between @param and @return. Not going to call out every instance in the file.
There are two spaces after the @param so that it lines up, but we don't do this.
Also, the @param docs don't seem to line up now that all parameters are being passed in as an object. I may be wrong on this. This may apply to:
Should the @private be removed too since we're now saying others can use Drupal.offCanvas?
This line needs to not come back.
Does this need to be on every page or can we be more selective? This is potentially a breaking change for sites since it would be a markup change for all sites without outside_in enabled (additional div tucked inside
<body>
).Comment #130
tedbowThis patch is against 8.4.x but without the revert of #2830882: Introduce Drupal.offCanvas, mirrored after Drupal.dialog, to avoid Settings Tray using Drupal.Dialog in unintended ways so it won't apply until that is reverted back.
1. fixed
2. fixed
except
I don't see this. Maybe someone else will.
3. Yes removed private
4. removed
5. Yes it needs to be on every page because off-canvas would be part of the regular dialog api it could be used on any page. Even if it wasn't there when the page loaded new content via ajax could have a off-canvas dialog link.
Comment #131
tedbow#2830882: Introduce Drupal.offCanvas, mirrored after Drupal.dialog, to avoid Settings Tray using Drupal.Dialog in unintended ways committed reuploading patch.
UPDATE:
interdiff-127-130.txt still applies here.
I just applied #127 and then that interdiff
Comment #132
GrandmaGlassesRopeManFrom #129.3,
I think if we want to document the destructing of the pseudo-object we can do something like that. Overall the JavaScript looks good; this mostly is just moving things around. 👍
Comment #133
star-szr@drpal thanks, yeah I'm really not sure how we document things like that. I'd say we can leave it as is and figure it out later :)
This line needs to be removed from stable CSS especially. (#129.3)
Just curious, why is this line being removed?
system_element_info_alter()
part should probably be mentioned in the change record, not sure it's going to catch people's attention but better than nothing. I'm guessing the main canvas wrapper is inserted server side because inserting it with JS is problematic in some way or less performant or similar?Also, tagging for manual testing.
Comment #134
xjmI see that @tedbow moved this patch back to 8.4.x above, but that should not have happened. After the alpha, committers evaluate issues for backport after commit. We don't ever guarantee that a certain issue will go into a certain branch.
Moving this one to 8.5.x now (again). Thanks @Cottser for the review!
Comment #135
GrandmaGlassesRopeMan@Cottser Yeah. There is surprisingly little information about JSDoc and destructuring objects in functions. I'll perhaps open a followup just to have a place to figure this out for the future since we've already used this pattern in a few other places.
Comment #136
tedbow#134 sorry moving to 8.4.x I don't think it was intentional(unless I gave a reason can't find comment)
Comment #137
tedbow#133.1 Remvoed. Now
diff core/themes/stable/css/core/dialog/off-canvas.form.css core/misc/dialog/off-canvas.form.css
is empty2. This is a mistake and out of scope.
^^ I double checked patch am about upload and "cursor" is not in it at all.
3. will look at change record
Comment #138
tim.plunkettThe only addition is
$this->getSession()->wait(800);
in the test, the rest are file moves, @internal removals, or s/tray/off-canvas dialog/I also manually tested the patch by using this with the Layout Builder (after uninstalling Outside In) and it works great.
Comment #139
tedbow@tim.plunkett thanks for review 🎉.
#133.3 I updated the change record https://www.drupal.org/node/2896596/revisions/view/10583796/10602357
Comment #140
star-szrThank you @tedbow, the changes look good. And thanks @tim.plunkett for reviewing and testing.
From #133.3:
Could someone please respond to this question? I'm not that worried that the markup change will break people's sites but I have a feeling the folks who like their markup clean probably won't love it.
Comment #141
droplet CreditAttribution: droplet commented@Cottser raised a good question. I always thought `[data-off-canvas-main-canvas]` inserted by outside_in and to avoid particular problems. (e.g. @see setEditModeState in outside_in.es6.js)
There're some new problems I've seen:
1. When it moves to CORE, shouldn't we apply padding to BODY by default? (In fact, shouldn't it apply margin?)
2. Could we call it off-canvas? It changed the main content width. Usually, the off-canvas is a overlay or shift the main content to left/right (with [negative] margin or transform).
3. BODY tag height =/= [data-off-canvas-main-canvas] height. the new wrapper might cause many unexpected issues
4. Shouldn't we add a new class `.off-canvas-opened` to BODY?
5. It could be a headache to a site has a complicated background image across the site elements or use CSS position for elements.
for exmaple, here's one of my client site. It coded with a very common HTML/CSS pattern:
(RED BOX = dialog)
Comment #142
tedbow@Cottser thanks for our continued work and vigilance on this!
#133.3 I guess don't see the advantage to inserting it via JS
"markup" here is not just what the server-side produces right? I mean if we insert it via JS it is still markup on the page. Would anybody who doesn't like the idea of the extra div like it any better if we inserted via JS.
Also think it would be much more disruptive if we add via JS. I think the only advantage to adding via JS would be that we could add it conditionally but then that would have some major problems:
Because of the above reasons the extra div wrapper would much less discoverable. Hopefully contrib and modules in core will start to use the off-canvas dialog. But likely modules would only need the off-canvas dialogs in certain situations so if we only add the div when there is any off-canvas link is on the page then there is the chance site developers won't encounter if the situation in their testing where the off-canvas wrapper is inserted via JS.
Considering a lot of people are not going to read release notes I think this would make it more likely custom theme developers would miss the chance to make sure the extra div is not disruptive if it was added conditionally.
I adding the div wrapper conditionally or on every page via JS could also cause a slight flicker when it is added if the div wrapper makes any visual change at all to the page. Similar to problems that have happened with the toolbar. Even if this didn't happen in core it would be 1 extra thing themes have to deal with.
Comment #143
tedbowI add an old tab open didn't have @droplet's comment there so I didn't remove the CSS tag intentionally
Comment #145
GrandmaGlassesRopeMan- #141.4 - In
beforeCreate()
we already add a class to the body,js-tray-open
.- #141.1,2,3,5 - I think these should be up to the theme to solve with the provided body class.
Comment #146
tedbowHere is just reroll
re #141
I agree #145 I think
js-tray-open
will allow theme to do what @droplet is asking for in #141 but I think it would be too much for core to try to solve this problem because we could then look at trying to image other things that themes would do and solve for different situations. I would rather do less and not create CSS that might mess up other situations.Comment #147
tedbowChanging to new settings_tray.module component. @drpal thanks for script help! :)
Comment #148
tedbowSettings this back to RTBC because it is just a reroll of an RTBC patch.
Comment #150
tedbowDuh, forgot about #2803375: Rename Outside-in module to "Settings Tray" for real which of course is going to make this need a re-roll.
Not assigning myself now because I am not about to work on it right this second so don't want to stop anybody else who can jump on it right away.
Comment #151
tedbowThis is re-roll of 2784443-145.patch. Because #2803375: Rename Outside-in module to "Settings Tray" for real landed I took .patch file and did a search and replace in the file for outside_in -> settings_tray, outside-in -> settings-tray, OutsideIn -> SettingsTray and outsideIn -> settingsTray. This allows the patch file to be applied with only few files that didn't apply and needed manual updates to the files:
Comment #152
tedbowLatest issue status updates
#138 = RTBC
#144 bumped to Needs Works because didn't apply
#146 Reroll
#149 bumped to Needs Works because didn't apply
#151 Reroll
So back to RTBC because nobody has set it to Needs Work only patches that didn't apply because of other commits
Comment #153
xjmAdding this to a queue of issues specifically for the frontend framework managers. Thanks!
Comment #154
tedbowNeeds a re-roll again for changes in
#2821724: Create Javascript Tests for Contextual Links and #2902191: Determine cause and fix random fail in \Drupal\Tests\settings_tray\FunctionalJavascript\SettingsTrayBlockFormTest::testBlocks
These only had changes in SettingsTrayJavascriptTestBase and SettingsTrayBlockFormTest so probably not too bad.
Comment #155
tedbowre-rolled
Comment #156
tedbowjust rerolled in #155
Comment #157
lauriiiI'm happy with the patch mostly. My only concern at this point is that dialog-off-canvas__main-canvas class name is not named correctly since there's no dialog-off-canvas. We can still change this but after moving this to stable we couldn't, so please, let's change this before moving there.
Just a nit but since I'm going to push back on this anyway, this could be made much easier to read using array_filter
Sorry for pushing back on this, I know the feedback loop is long.
Comment #158
tedbow@lauriii thanks for taking a look here again.
1.
I am not sure what else to name it.Incore/modules/system/templates/off-canvas-page-wrapper.html.twig
we have
<div class="dialog-off-canvas__main-canvas" data-off-canvas-main-canvas>
So for the class name,dialog-off-canvas__main-canvas
we have 2 partsdialog-off-canvas
which tells main concept this relates to is the off-canvas dialog. But "off" from what? We could rename itdialog-off-canvas__canvas
but for me that would imply that this is the "canvas" portion of the "off-canvas" dialog. "main-canvas" to imply that this is the "main" thing that dialog separate from.I am open to suggestions.UPDATE: actually I think just changing
dialog-off-canvas__main-canvas
todialog-off-canvas__page-wrapper
make sense. It is more descriptive. So also changingdata-off-canvas-main-canvas
todata-off-canvas-page-wrapper
.2. Nice catch, fixed.
Obviously i would love to get this in but I appreciate the diligence of you and @cottser on this issue.
Comment #159
tedbowI chatted with @lauriii and I misinterpreted comment #157.1\
I was referring to use of "__" which is BEM pattern that is misused here.
So in this patch I am changing to
dialog-off-canvas-main-canvas
and no other changes related to that.Comment #160
tim.plunkettChanges since RTBC look good!
Comment #161
timmillwoodWe're looking to use the off canvas dialog in Workspaces module via #2916781: Allow off-canvas dialog to be rendered at the top of the page.
Once, this issue is in it'll mean we won't need to depend on Settings Tray module.
Comment #163
tedbowThis needed a reroll because of #2870456: Convert web tests to browser tests for Settings Tray module
This is steps I took
The interdiff provided is from step to 2 to 3.
Comment #164
tim.plunkettThis is an interesting change, but seems fine
Comment #166
tedbowre #165
There was random failure in
\Drupal\Tests\editor\FunctionalJavascript\EditorFilterFormConfigurationTest::testSwitchingEditors
which was just created today in #2701393: Switching between editors on the format configuration causes errors upon saveWill investigate
Tested again and it passed.
Comment #167
star-szrShort on time at the moment but I took a look at this for a few minutes and it's looking good :) I'm +1 to commit this.
Comment #168
larowlanAdding review credits
Comment #169
larowlanCommitted as c586b21 and pushed to 8.5.x.
Published the change record.
Comment #171
tedbow🙏🏽
@larowlan, @Cottser, @lauriii thanks for time and effort on committing this!!!!
🙌
@Wim Leers, @tim.plunkett @nod_ @xjm @drpal thanks for all the help!!!
🎉🔥🎉🔥🎉🔥🎉🔥🎉🔥🎉🔥🎉🔥🎉🔥🎉🔥🎉🔥🎉🔥🎉🔥🎉🔥🎉🔥🎉🔥🎉🔥
🎉🔥🎉🔥🎉🔥🎉🔥🎉🔥🎉🔥🎉🔥🎉🔥🎉🔥🎉🔥🎉🔥🎉🔥🎉🔥🎉🔥🎉🔥🎉🔥
Comment #172
Wim Leers🤣