Nested Off-Canvas II #10003
Nested Off-Canvas II #10003
Conversation
|
@SassNinja thanks for putting together a clean PR! |
|
@SassNinja sorry for the delay in looking over this! Thanks a lot for putting this together 😄 So firstly, from what I understand the aim of this PR is:
First thing I noticed is the One thing I'm not sure about is making the From the old work I did here: http://brettmason.co.uk/offcanvas/content-option/foundation.offcanvas.js you will see I took a slight different approach. Get the position class from the I'll continue to look over it as I haven't looked at off canvas for a few months so am a bit rusty again 😄 Have a look at what I put together too and see if any of that code could be used to perhaps simplify the approach a bit. Good job so far though 👍 |
|
@brettsmason thanks for your feedback so far! You're right regarding the aims of my PR. Several times I have been facing the problem I need some parts of the page to be usual content for desktop and move into off-canvas for medium down (without duplicate content!). The things you noticed:
Apart from that I've checked you're code. But compared to your approach I've used the position class (not the transform property/matrix) and I haven't only limited the content part to the open method but initialized the content as this.$content in the setup. My intention was to fully integrate the content.
When you test my latest code you'll probabely notice some more issues: Issue 1 Issue 2 Apart from that I'm quite happy with my code as it doesn't seem to break existing code (backwards compatibility was important to me) and offers much more flexibility for 'hardcore off-canvas users' cause it lets you also move several elements into the same off-canvas side. Regarding your code I haven't spotted something important that I've missed or should take over (at most the naming). In case I'm missing anything, just let me know. I'm looking forward to getting more feedback from you once you've done reviewing my code and once the derusting is finished ;) |
|
@SassNinja I've pulled that down and the bug I mentioned is now gone 👍 To answer your points:
I'm not totally sure at this point that its going to be possible to get a cross browser compatible version that allows nesting flawlessly. Removing the siblings reliance is definitely a good idea though. Version 6.4 should be released in June, so would be good to get this included in some form! |
|
@brettsmason yeah, I worried about that. I agree regarding the benefits of transform: performance is definitely more important! I've adjusted the JS now so a fixed position gets emulated for a nested off-canvas element (by setting height & top value based on the viewport's height and current scrollTop). The fixed issue seems to be solved now in my eyes. I've also added a new inner box shadow for a (nested) push off-canvas element because the ::after solution doesn't work if the element's height is greater than the viewport's height (non-nested elements seem to have the same issue). The only issue that remains is the IE issue. I haven't found a solution for this yet but I'm working on it. June as release date sounds good to me 👍 |
The height should not only be reset if the element is open but every time so the value is correct on opening.
Since position fixed doesn't work for nested off-canvas elements, it has to be position absolute. Surprisingly (not) the IE doesn't show an opened off-canvas element unless absolute is explicitly set.
|
I think I've got it! p.s. |
Due to the IE fix where position absolute gets explicitly set, in-canvas has to use !important to override this reliably.
|
@brettsmason did you already manage to review my latest code? |
|
@SassNinja Apologies been working on getting the new grid sorted out and forgot! |
| & ~ .#{$content} { | ||
| margin-#{$position}: $offcanvas-size; | ||
| } | ||
| } | ||
|
|
||
| /// Overrides the off-canvas styles | ||
| @mixin in-canvas() { |
There was a problem hiding this comment.
Is there a need for !important here? Generally we try and avoid the use of this whenever possible.
There was a problem hiding this comment.
I've used !important here due to the CSS specificity of the things I'm overriding.
Otherwise every property I've set important wouldn't work with only one simple in-canvas selector.
I would have to fight against the specificity of the following selectors:
- transform => .off-canvas-content .off-canvas.position-[DIRECTION]
- visibility => .off-canvas.is-closed
- height => inline style set by JS (fixed position emulation)
- box-shadow => .off-canvas-content .position-[DIRECTION].is-transition-push
- position => .off-canvas-content .off-canvas
I agree !important is not cool but I think in this case it's better than including in-canvas into all the long selectors (inline style is even impossible without important).
It's worth to mention I only used !important where necessary. Some properties of my in-canvas feature are working without it (background, width, overflow, transition) and just use 'inherit'.
|
@SassNinja I've tested the updates you have made in a variety of browsers, including IE9. I'm pleased to report everything works great 👍 A few points/questions:
I'm going to invoke the power of @kball to take a look over the JavaScript as hes far better than me with that 😆 |
js/foundation.offcanvas.js
Outdated
| if (this.options.contentId) { | ||
| this.$content = $('#'+this.options.contentId); | ||
| } else if (this.$element.siblings('[data-off-canvas-content]').length) { | ||
| this.$content = this.$element.siblings('[data-off-canvas-content]'); |
There was a problem hiding this comment.
Does this need a .first() as well?
There was a problem hiding this comment.
- ID doesn't make sense since there shouldn't be more than one match
siblings('[data-off-canvas-content]')didn't usefirst()in v6.3 so the behavior wouldn't change when upgrading (backwards compatibility)closest('[data-off-canvas-content]')usefirst()but I think it should behave identical tosiblingsso either both use it or none
Since this.$content should only match one element I think it's a good idea to make this sure by using first(). I'll adjust this!
I can't imagine someone is expecting a match of two or more content elements here (as it was possible in v6.3) what doesn't make sense. So backwards compatibility shouldn't be problem here.
js/foundation.offcanvas.js
Outdated
| } | ||
|
|
||
| // Assume that the off-canvas element is nested if it isn't a sibling of the content | ||
| this.nested = this.$element.siblings('[data-off-canvas-content]').length && !this.options.contentId ? false : true; |
There was a problem hiding this comment.
I'm confused here... what is this trying to do? First off, since our outcomes are booleans I think this would be easier to read if we just invert the logic and get rid of the ternary operator, so:
this.nested = this.contentId || !this.$element.siblings('[data-off-canvas-content]');
That said, I don't understand why this is true... we're always nested if we have a contentId? What is the exact case we're trying to account for here?
There was a problem hiding this comment.
I'm using this.nested to emulate a fixed position via JS (see _emulateFixedPosition()) and force some options (e.g. no content scrolling) since this is necessary when nesting the off-canvas.
But it is quite difficult to determine if an off-canvas is nested – I don't want to parse the whole DOM for this!
That's why I assert the element is nested if it has no siblings content. But the usage of a contentId is a problem here since the related content element may be placed anywhere on the page (can be a sibling, can be nested, can be none of this).
Therefore I'm asserting that the off-canvas is nested if the contentId option is used so the code can probably simplified as it's probably not realistic to have multiple content elements at the same level:
this.nested = !this.$element.siblings('[data-off-canvas-content]').length;
Of course this limits the flexibility of using the contentId but I don't know a good way to check if the content element with the target Id is nested.
What do you think?
There was a problem hiding this comment.
Hmm... well we've already found $content, so can we simply compare $content[0] to this.$element.siblings('[data-off-canvas-content]')[0] ?
| @@ -65,9 +85,9 @@ class OffCanvas extends Plugin { | |||
| overlay.setAttribute('class', 'js-off-canvas-overlay ' + overlayPosition); | |||
| this.$overlay = $(overlay); | |||
| if(overlayPosition === 'is-overlay-fixed') { | |||
| $('body').append(this.$overlay); | |||
| $(this.$overlay).insertAfter(this.$element); | |||
There was a problem hiding this comment.
At the beginning of this feature I had some issues with the z-index / overlapping of the overlay elements what was gone after this adjustment. I've just rechecked this and is doesn't occur anymore when I undo the adjustment so it's probably not necessary anymore.
Do you prefer the way it was done in v6.3 meaning to append it to the body?
I personally find it quite pleasant to have the overlay close to the related element but that may be a matter of taste.
There was a problem hiding this comment.
My concern about having the overlay close rather than after the body is that unless I'm mistaken any sorts of transforms applied up the tree will break the fixed positioning
|
Left some feedback on the JS... agree on @brettsmason's questions as well, but broadly speaking this is looking very good. Nice work @SassNinja! let's get this across the finish line for 6.4 (first RC targeted for next Thursday) |
|
@brettsmason @kball many thanks for your detailed feedback! I've answered your questions in the appropriate review notes.
@kball |
Since it doesn't make sense to have more than one match for the content element I'm using first() to make this sure.
Top position mustn't get any inline height (preserve CSS height) but only an updated top value. Bottom position is identical to top position but needs another calculation to pseudo fix it to bottom.
Using a pseudo ::after element for the off-canvas push panel shadow causes issues if the element's content height is greater than the element's height. Therefore I've added two additional variables for the push panel and use it to create an inner box shadow.
Regarding the off-canvas docs setup, a detection of the prior off-canvas wrapper inner is necessary. Besides I'm fixing some specificity issue caused by _page.scss (foundation-docs)
|
@kball you're right, I didn't notice that new issue on the docs page – sorry for this! I'll do more testing when working on this feature. Discarding the push transition if nested is not an option for me. I've now worked over the whole code and (as suggested by you) forced the overlap transition if nested. Apart from that I've noticed some issues that also occur on the live page:
@brettsmason @kball would be great if you can take a look at the issues I've listed above. |
|
@SassNinja To answer your points:
I'll let @kball come back to you on the other stuff, but generally no need to worry about the docs, so anything you added to do with the inner wrapper etc because of the docs can be removed as its not needed 😄 |
Since wrapper inner is deprecated and erroneously implemented in the docs, I'm undoing my fix. Once the docs got fixed, the push transition is going to work as expected.
Firstly the implementation was erroneous (content mustn't be a sibling of the wrapper but a child) and secondly the wrapper inner is deprecated. Therefore I'm removing it what makes the push transitions on the off-canvas docs page working again as expected.
But as previously mentioned the fixed top bar on the docs page looses its fixed state now once a push transition element gets opened. @brettsmason what do you think, shall I always set I mean the fixed state would still get lost (I remember our initial talk about fixed & transform) but at least it will be restored if closed. Some words regarding no. 3 of my list: @mixin off-canvas-base(
// Set the off-canvas z-index.
z-index: $offcanvas-push-zindex;
// Increase CSS specificity
&.is-transition-push {
z-index: $offcanvas-push-zindex;
}
} |
…ault transform none for content Now the content container has none transform by default so position fixed works as soon as an push off-canvas is closed. Besides the transitionend listener is now fixed so it isn't bound to only the content or the element but to both of them. The content classes specification is now part of the setup (this.contentClasses) for an improved readability.
…orrectly Due to the content's initial transform:none the transitionend listener didn't work for the content container. This is fixed now by using the has-transition-push class that is also used in the positions mixin to limit the translate to push transitions.
Since the limited version doesn't enable push transitions for nested elements, there's no need to listen on the content because the element always gets moved.
|
Update: I've worked over the add/remove content classes part now and set So for me the limited version of this off-canvas feature looks good. Would be great if you can review it and let me know whether you spot any issues I've missed. Apart from that I'm having some problems with the transitionend listener in another feature branch (not affecting this limited version – I'm justing working on a nested push transition support for the future): Apart from that I'm facing an issue with the
@kball maybe you can help me with this |
|
@SassNinja just checked this out, still seeing some of the docs examples not working, in particular the examples under Off-canvas Directions don't appear to work in this branch. It also seems to break the push examples in the sticky visual test... Unfortunately with just over 1 week to final 6.4 release, I think this is looking very likely to slip out to 6.5 |
|
@kball so the directions chapter is the only 'erroneous' part in the docs, isn't it? But to be honest I'm not sure if this behavior makes sense. I can't imagine why you would nest a content in another content. Besides it looks weird (in my fork & live) when the nested content and its elements doesn't act as the usual off-canvas (whitespace between both). Regarding the off-canvas sticky visual test: The sticky bar looses it's fixed state on opening a push element and gets it back once the element is closed. As mentioned in my previous posts this is caused by the transform scope the push transition creates. It's a pity if at least my limited feature won't be part of 6.4 but I understand you want to be on the safe side. |
|
I do hope this will find it's way to 6.4 at least in the minimal form. I could use the functionality as I have pretty similiar usecase as described in the initial post #9988 |
|
@SassNinja you are correct, I apologize, I reconfirmed that sticky visual test on prior releases just now and the behavior is the same. @brettsmason can you take one final look through this and lets see if we can get this merged? |
|
@SassNinja Really sorry I've been MIA - head stuck in the XY grid. So pulled down the latest changes and have been looking over it for any bugs I can find:
👍 though its looking good overall. |
| z-index: $offcanvas-push-zindex; | ||
|
|
||
| // Increase CSS specificity |
There was a problem hiding this comment.
What's the purpose of this? Don't fully understand why its needed.
There was a problem hiding this comment.
I've done this to override the fix z-index that is set in https://github.com/zurb/foundation-docs/blob/master/scss/_page.scss
If this gets removed we'll be able to undo this specificity fix.
| @@ -154,10 +185,20 @@ $maincontent-class: 'off-canvas-content' !default; | |||
|
|
|||
| transform: translateX(-$size); | |||
| overflow-y: auto; | |||
|
|
|||
| // Sets the position for nested off-canvas element | |||
| @at-root .#{$maincontent-class} .off-canvas.position-#{$position} { | |||
There was a problem hiding this comment.
This seems to be the route cause for the need for !important usage. You have a better understanding of the way nested works though, but is there no way we could restructure the Sass a bit to make this less specific? I understand it may not be possible.
There was a problem hiding this comment.
I was facing a lot specificity problems when mixing push & overlap with the nested feature.
But since the push support for nested elements is removed (for now) I guess this may get simplified a bit.
Would you like me to work over it and see what I can cut down for this limited version?
Since the push support for nested elements got reverted the height doesn't have to fight inline CSS anymore. Box shadow and transform override work with a increased CSS specificity now.
js/foundation.offcanvas.js
Outdated
| Keyboard.releaseFocus(this.$element); | ||
| } | ||
|
|
||
| // Listen to transitionEnd and add class when done. | ||
| this.$element.one(Foundation.transitionend(this.$element), function(e) { |
There was a problem hiding this comment.
This will need to change to using an import rather than Foundation global (see https://github.com/zurb/foundation-sites/blob/develop/js/foundation.util.motion.js#L4 for example)
but if you like I can do this at merge time.
There was a problem hiding this comment.
done now in 5aa588d
|
I think with those last changes this is probably good to go! @brettsmason what do you think? I'm out of office today but should be able to merge & turn around the RC tomorrow morning. |
|
@kball If we can solve issue 2 above and hide the close button when an off canvas is in-canvas (like |
Since reveal-for-x uses now CSS for hiding the related JS for the inline style is removed.
…ation global with imported transitionend
|
@brettsmason thanks for the feedback & review! Regarding your bug list:
@kball I've adjusted this as requested by you. It's part of this commit 5aa588d |
|
@SassNinja Thanks for taking another look! To answer your points:
Great work and thanks for being so patient and thorough 👍 @kball from my point of view this looks ready to merge now. You might want to check the JS changes quickly but I'm happy with the Sass. |
|
All right! Scanned the JS again and happy with it as well. Merging. Last major feature change to sneak into the 6.4 release :) Nice work @SassNinja! |
As discussed in the original PR (whose history got messed up) I've recommitted everything and opened this new PR.
For reference check the original post:
#9988
@brettsmason again, would be great to get your feedback on my code :)
Best regards
Kai