gma / nesta

File Based CMS and Static Site Generator
http://nestacms.com
MIT License
902 stars 121 forks source link

Add breadcrumb options #118

Closed gadomski closed 11 years ago

gadomski commented 11 years ago

This branch adds a couple of options to display_breadcrumbs. I added all these options so I could make Nesta's markup play well with Bootstrap.

Cheers.

gma commented 11 years ago

I've never used Bootstrap. Is there no way you could style the active breadcrumb (which has to be the last one) with CSS selectors (e.g. :last-child)?

I'd also have thought dividers (which are presentational, rather than semantic) could be handled with CSS :before or :after selectors. Is there anything in Bootstrap that prevents that?

gadomski commented 11 years ago

Nothing in Bootstrap prevents you from using css selectors to achieve the same presentation. The motivation behind the PR comes from the desire to tweak my HTML layout instead of writing new style, which is a personal preference rather than true requirement.

I noticed in #120 you mentioned a nesta-bootstrap plugin, which seems like a reasonable way to get these changes without cluttering up the main codebase with non-essential options. I'll take a look at rolling these three (#118, #119, #120) into a plugin so we can see if that's a less invasive way to go.

gma commented 11 years ago

Nothing in Bootstrap prevents you from using css selectors to achieve the same presentation. The motivation behind the PR comes from the desire to tweak my HTML layout instead of writing new style, which is a personal preference rather than true requirement.

Could you clarify something for me? Are you saying that #118, #119 and #120 are required in order to use a default version of Bootstrap, without modifying it or writing any more styles? If these changes are all you'd need to get Bootstrap hooked up perfectly with Nesta, my plugin suggestion is a reasonable one. If, on the other hand, it's just one possible way to use Bootstrap my suggestion of making a plugin out of these things is a rather bad idea.

That's because its presence would suggest to people that they'd need to use the plugin in order to use Bootstrap, whereas in fact they'd be quite alright by adding a bit of CSS. They'd be better off without the plugin (as there'd be fewer moving parts) but might waste time trying to work out whether they needed it.

If any changes to Nesta are required to be able to use Bootstrap, those are the only changes that ought to go in a plugin with bootstrap in the name.

Is there a specific bit of Bootstrap that I could look at that would help me explain how it's supposed to be used here, or how it styles breadcrumbs, for example? I'm aware that having no Bootstrap experience is holding me back a bit here…

gadomski commented 11 years ago

Here's Bootstrap's example of how to mark things up for their breadcrumbs. They also do nav lists (scroll down a bit to get to the nav list part).

I see breadcrumbs and navigation lists as two parts of Bootstrap's display suite that could fall nicely into place for a Nesta project. Right now, I can't just drop Bootstrap's css into my project and use it to style my Nesta breadcrumbs and menus. I have to write my own styles (and/or Nesta customizations) to get Bootstrap's look-and-feel in a Nesta project. I see that work (the custom styles and/or Nesta tweaks) as the content of the plugin. Maybe nesta-plugin-bootstrap-helper to avoid the 'Do I have to use this' confusion?

gma commented 11 years ago

Okay, that makes sense, thanks. I like the new helper name suggestion.

I'm up for defining a current_menu_item_class method too, so the plugin (or any site) can override it. Should probably have a unit test for it as a reminder that it's public API.

Same for the breadcrumb if that would make sense. Probably better to use "current" than "active" for the methods, for consistency.

gadomski commented 11 years ago

Bummer. I played with all that rspec for about an hour and can't figure out how to write an spec for a module-method override in page_spec.rb for current_menu_item_class. I tried using stub! and also explicitly opening the module and monkeypatching, but to no avail, I can't seem to tweak the output of the module method. If you need to see this tested, would you be able to provide a code example of how to test current_menu_item_class? I'm bummed I can't solve this on my own but I'm stuck...

-notarubyguy

gma commented 11 years ago

Okay, no problem. Just send a new pull request with the new hook methods in and I'll take a look at it.

gadomski commented 11 years ago

Hooks added in #121. I'm closing this pull request since we won't be using this branch anymore; let me know if you'd prefer to do the closing in the future?

gma commented 11 years ago

Feel free to close PRs yourself if you've found a better way to go with something…