Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add experimental off-canvas layout #6565

Closed
wants to merge 21 commits into from

Conversation

@trumbitta
Copy link
Contributor

commented Jan 12, 2013

I don't think this is going to ever be accepted, but I hope it could be a nice beginning to play with. Just read on...

Based on:
https://github.com/bradfrost/this-is-responsive/blob/gh-pages/patterns/layout-offcanvas-right.html

TODO:

  • fix WebKit infinite loop transition bug (automagically fixed since Bootstrap 3 switched to Mobile First)
  • refactor JS into a proper Bootstrap plugin (but can be improved), with proper unit tests
  • must be tested on all the browsers supported by Bootstrap (this is a WIP and will be unchecked whenever there's some related bug being worked out)
  • add proper documentation
  • add variant with sidebar coming from the left: .row-offcanvas-left
  • delete temporary html file from examples
trumbitta added 2 commits Jan 12, 2013
Based on:
https://github.com/bradfrost/this-is-responsive/blob/gh-pages/patterns/layout-offcanvas-right.html

It lacks a nice transition effect due to a strange bug happening
only in Chrome at certain viewport sizes.

Other gotchas: only works with a sidebar coming in from the right,
and the required javascript is ugly and written in the HTML file
because I don't feel comfortable enough to figure out how to write
a noob-level plugin for Bootstrap.
@mdo

This comment has been minimized.

Copy link
Member

commented Jan 13, 2013

If you can clean this up and isolate the custom CSS to the page, we can add this as an example. None of the CSS should be in the /less/ directory, the JS should be cleaned up and bugs worked out, etc. Build it up to something usable and stable, and we'll do our best to get it in there.

@blakeembrey

This comment has been minimized.

Copy link
Contributor

commented Jan 14, 2013

@trumbitta Do you have an example of the transition bug. I've been using an off-canvas layout for http://requestmaker.jit.su/ to play around with, and haven't seen any issues with the transition.

@trumbitta

This comment has been minimized.

Copy link
Contributor Author

commented Jan 14, 2013

@blakeembrey I'll try to put one up asap.

@mdo Thank you, I'll do my best to follow your directives.
Just a question: do you think that Bootstrap should not have an off-canvas component / option, and this is why you want this as an example with its own CSS and JS?

My first thought was to end up with an addition, maybe a JS component, and I was starting to look for help from a friend with the JS part with the aim to write a real plugin.

@mdo

This comment has been minimized.

Copy link
Member

commented Jan 14, 2013

@trumbitta If it's not 100% compatible and stable, it should be an example. If it's something you can write in the exact same style as our JS plugins, document, and support for IE8+, then we can consider adding it to the core. If you want to do the latter, be sure to read the Contributing doc and the JS docs.

Thanks!

@trumbitta

This comment has been minimized.

Copy link
Contributor Author

commented Jan 14, 2013

@mdo I'm trying to get there with the JS plugin (see the commits coming in the next minutes).
I'd like to keep updating this pull request while trying to reach a usable state, to get some feedback / hints / help / whatever while I'm working on it.
Of course, if you prefer, I could aswell work outside a pull request and come back when it's "perfect"ish.

@blakeembrey here's the transition bug: https://dl.dropbox.com/u/21966344/offcanvas-bootstrap-transition-bug/index.html

  • I'm using Chrome Version 24.0.1312.52 on Linux
  • resize your window at 767px, maybe less. Stop resizing when the blue button shows up on the top right of the jumbotron element, and do not make your window too much narrow because the bug disappears after a while.
  • please, ignore the missing icon on the button
  • push the button, behold the crazy loop
TODO:

 * JS unit tests
 * fix the transition bug at ~767px (infinite loop)
 * remove examples/off-canvas.html, replace with proper documentation
 * refine, polish, refine
@blakeembrey

This comment has been minimized.

Copy link
Contributor

commented Jan 14, 2013

@trumbitta I just updated to Chrome 24 on Ubuntu and aren't seeing the bug. Don't suppose you have any more details?

@blakeembrey

This comment has been minimized.

Copy link
Contributor

commented Jan 14, 2013

@trumbitta Lol, ignore that. Just found it, I see what you mean now. Other than that though, it looks great.

@Yohn

This comment has been minimized.

Copy link
Contributor

commented Jan 14, 2013

@trumbitta that looks nice, I've been playing with it in firefox 18 on windows xp and it seems to be working correctly from what I can tll.. I couldnt see any bugs happening, it opens and closes when you click the icon-less button.. I really look forward to seeing something like this added to bootstrap!

@trumbitta

This comment has been minimized.

Copy link
Contributor Author

commented Jan 16, 2013

I put some hours in debugging that transition, but wasn't able to fix it.

To my tests, the issue is somewhat similar to http://stackoverflow.com/questions/12363172/how-to-avoid-infinite-loop-with-css3-transition-and-vertical-scroll-bar-in-webki

If you set two different background-color properties inside and outside the 767px threshold, and increase the transition-duration to - say - 5s, you can observe quite clearly what's going on:
When the .row-offcanvas wrapper starts its transition towards right: NNpx, Chrome gets confused about what media query is to be applied at a given time, and falls in a infinite loop.

At this point, my solution would be to remove the transition. Would anyone else like to take their chances on this juicy little bug?

In the next days, I'm going to test the pull request against IE8+ and replace the example HTML file with some proper docs.

@blakeembrey

This comment has been minimized.

Copy link
Contributor

commented Jan 16, 2013

@trumbitta I'll take a look, it should be easy enough to work around. Have you tried floats and negative margins yet? I used this approach and haven't hit this bug yet. I am using it on http://requestmaker.jit.su/ and the code is here https://github.com/blakeembrey/requestmaker/blob/master/app/public/styl/requestmaker.styl

@trumbitta

This comment has been minimized.

Copy link
Contributor Author

commented Jan 17, 2013

@blakeembrey Thank you!

I tried some floats and negative margins, but not extensively.
One of my goals is to respect, and use as a foundation, the core grid behaviours (i.e. no float < 767px) and the landmark viewport width values for the media queries.

@trumbitta

This comment has been minimized.

Copy link
Contributor Author

commented Jan 17, 2013

Thank you god and @mdo for the switch to Mobile First. That single move nuked away the transition bug!

Wait for the related commits sometimes today :)

@trumbitta

This comment has been minimized.

Copy link
Contributor Author

commented Jan 19, 2013

@mdo except for the HTML file in the examples (which is still there just for a quick and comfy test while working on this, and would be removed before the end), I think the first complete iteration is finished.

I'm looking forward for your thoughts / directives on further refinement or whatever.

@steffler

This comment has been minimized.

Copy link

commented Jan 24, 2013

@trumbitta I just wanted to chime in and thank you for work on these commits. This is exactly what I've been trying get bootstrap to do for the past week after I came across Brad Frosts' work.

When it comes to writing JS, I am about as green as it gets. I'm still very new to coding so I can't imagine how much time this has saved me.

@trumbitta

This comment has been minimized.

Copy link
Contributor Author

commented Jan 25, 2013

@steffler thank you, this is exactly the reason why I embraced Open Source a life ago 😸

var $this = $(this)
, $parent

if ($this.is('.disabled, :disabled')) return

This comment has been minimized.

Copy link
@fat

fat Feb 8, 2013

Member

don't need this – disabled property doesn't fire events

This comment has been minimized.

Copy link
@trumbitta

trumbitta Feb 8, 2013

Author Contributor

Indeed.
(Poorly executed) TDD misguided my judgement.
Commit incoming.

e.preventDefault()
e.stopPropagation()

if ($this.is('.disabled, :disabled')) return

This comment has been minimized.

Copy link
@fat

fat Feb 8, 2013

Member

don't need


if (e.keyCode == 27) {
if (e.which == 27) $parent.find(toggle).focus()
return $this.click()

This comment has been minimized.

Copy link
@fat

fat Feb 8, 2013

Member

call toggle here directly, not a click event


$parent.hasClass('active')

if (e.keyCode == 27) {

This comment has been minimized.

Copy link
@fat

fat Feb 8, 2013

Member

confused a bit here… but shouldn't e.keyCode and e.which be the same?

This comment has been minimized.

Copy link
@trumbitta

trumbitta Feb 8, 2013

Author Contributor

the whole keydown block it's a botched attempt at being smart when using a keyboard...

On second thought, I'd go for an extremely light first version:

[...]
keydown: function (e) {
      $(this).toggle
}
[...]

This should be enough to provide the basic functionality, while proceeding further - maybe next version? - I'd like to have the following behaviour:

  • on active via keyboard, give focus to the sidebar
  • while in sidebar: ESC gives back focus to the toggler anchor/button

I don't feel skilled enough now, but I'm willing to improve myself and this plugin.

Your thoughts on my plan?

@ckluis

This comment has been minimized.

Copy link

commented Feb 22, 2013

@trumbitta THANK YOU

Off-Canvas can be extremely useful - especially with content that could be displayed dynamically. If you look at the
basecamp iPhone app (or actually even twitter.com) - the concept of slidein content works very well.

Have you considered creating a non-menu style solution for off-canvas? I believe a great implementation of off-canvas would be allowing the off-canvas area to be built via the same grid/column structure of the base theme with a new off-canvas css/javascript designator.

"off-canvas oc-left span3"
"off-canvas oc-right span4"
"off-canvas oc-top"
"off-canvas oc-bottom"

Unfortunately, I'm not much of a developer or designer or I would attempt to code it and provide you with a sample.

http://bradfrost.github.com/this-is-responsive/patterns.html - has good off-canvas content examples - the idea of the off-canvas items being hidden until selected is enticing

@trumbitta

This comment has been minimized.

Copy link
Contributor Author

commented Feb 22, 2013

@ckluis actually this plugin is inspired by This Is Responsive (have a look at the description of the pull request 😄 ).
Also, you're not forced to use a menu. The plugin should work with various types of inner content.

@ckluis

This comment has been minimized.

Copy link

commented Feb 22, 2013

@trumbitta Indeed, most excellent! I was trying to get the concept of a sizable off-canvas solution. I can imagine things like a contact form being off-canvas right - just an example.

@dharmaone

This comment has been minimized.

Copy link

commented Feb 27, 2013

how can i use this plugin?

@trumbitta

This comment has been minimized.

Copy link
Contributor Author

commented Feb 28, 2013

@dhelder check the pull request, there's also proper documentation in the docs. BTW, it is a bit outdated right now.

Waiting for a reply from @fat on my plans for the JS part, to move on and keep it up-to-date with the latest changes from upstream

@breerly

This comment has been minimized.

Copy link

commented Apr 10, 2013

Big +1 on this.

@mneuhaus

This comment has been minimized.

Copy link

commented Apr 17, 2013

+1

@dhelder

This comment has been minimized.

Copy link

commented Apr 17, 2013

@trumbitta I have nothing to do with Bootstrap.

@trumbitta

This comment has been minimized.

Copy link
Contributor Author

commented Apr 17, 2013

Lol sorry @dhelder . I was replying to @dharmaone

}

, keydown: function (e) {
console.log($(this))

This comment has been minimized.

Copy link
@fat

fat May 4, 2013

Member

assuming you don't want me to review this…

@fat

This comment has been minimized.

Copy link
Member

commented May 4, 2013

my feedback on the js is that it doesn't look like it should be a plugin… if you're jus toggling a single class.

@trumbitta

This comment has been minimized.

Copy link
Contributor Author

commented May 4, 2013

Ok, I think this means that I should have followed the very first advice by @mdo ( #6565 (comment) ) and make it an example.

Due to a bandwith issue I can't work on it at least till Monday.

Please, let me know if you guys want me to close this and open a new PR or anything else.

@trumbitta

This comment has been minimized.

Copy link
Contributor Author

commented May 6, 2013

I'm closing this, as it's now a standalone example. #7799

@trumbitta trumbitta closed this May 6, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.