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

Remove nav walker and Bootstrap navbar #1427

Merged
merged 1 commit into from Apr 29, 2015

Conversation

Projects
None yet
@retlehs
Copy link
Member

retlehs commented Apr 14, 2015

Cleaner walker moved to a Soil module: roots/soil#73 (it no longer includes support for Bootstrap dropdowns)

Remove the Bootstrap navbar and use a more generic header template (remember, this is a starter theme (and we're becoming framework agnostic))

image

@retlehs

This comment has been minimized.

Copy link
Member

retlehs commented Apr 15, 2015

once upon a time, the starter theme had two header templates:

  • BS top nav
  • a simple header (like in this PR)

the simple header was removed in favor of just the BS top nav about a year ago

i never liked this change. personally, i have never used the BS top nav in a theme and it's one of the first things i remove when starting a new project.

we don't need to necessarily remove this right now [as the generator isn't yet ready], but i'd like to see it go. as you can see, we can remove 173 lines of code from the theme by doing this.

any feedback/thoughts? do you use the BS top nav?

@JulienMelissas

This comment has been minimized.

Copy link
Member

JulienMelissas commented Apr 15, 2015

I'm all in favor of cleaning up Sage to be more framework agnostic, but I personally use the provided nav walker as a starting point for many of my project's navigation (when I'm using Bootstrap). I think once the generator is in, it should be an addition if someone chooses Bootstrap (we should also provide the default still, IMHO).

For now, it might be a nice idea to keep it as a good starting point? Maybe add the "basic" navbar file back in?

@jdcauley

This comment has been minimized.

Copy link

jdcauley commented Apr 15, 2015

I'm in favor of keeping it until the generator is there to replace it.
Not having to worry about the nav something I enjoy.

@retlehs

This comment has been minimized.

Copy link
Member

retlehs commented Apr 15, 2015

roots/soil#73 is still compatible with bootstrap navigation menus because it adds the active class. it just doesn't support dropdowns.

if you added the BS topnav markup to the header template and had soil activated, you'd be bueno if you didn't need more than just the top level nav.

@retlehs

This comment has been minimized.

Copy link
Member

retlehs commented Apr 15, 2015

with this PR, if you just add nav-pills on the menu class you get:

image

with this PR, if you re-add the BS top nav markup, you get exactly what you're expecting (assuming soil is activated. if it's not, active menu item highlighting won't work out of the box):

image

@retlehs

This comment has been minimized.

Copy link
Member

retlehs commented Apr 15, 2015

@JulienMelissas

but I personally use the provided nav walker as a starting point for many of my project's navigation

what do you need from it - the dropdowns? (also, are you not using soil?)


@jdcauley

Not having to worry about the nav something I enjoy.

what does that even mean? are you using dropdowns or what?

@mAAdhaTTah

This comment has been minimized.

Copy link

mAAdhaTTah commented Apr 15, 2015

Doesn't everyone use dropdowns? I think if you're building a theme, you have to take dropdowns into account if they're available on the backend. +1 on leaving this in until the generator comes.

@retlehs

This comment has been minimized.

Copy link
Member

retlehs commented Apr 15, 2015

Doesn't everyone use dropdowns?

erm, no.

and to re-iterate what i said earlier, this is a starter theme. there is plenty of stuff "available" on the backend that doesn't have any effect on the front-end.

@leopuleo

This comment has been minimized.

Copy link
Contributor

leopuleo commented Apr 15, 2015

I'm in favour of this cleaning up, but what about the BS collapsing navbar (and related button)?
This is maybe the first feature required from a mobile first theme.

Usually I override the default header with a custom one:
BS solution does not work in case of complex mobile header (menu button + search button + links) but in case of a simple header (logo + nav), a BS nav is honey...

I'm thinking about _s starter theme: the only default javascript is the one handles toggling the navigation menu for small screens. 😄

After the discussion listed under #1258 I tried to make Roots/Sage header compatible between different frameworks, using the same HTML structure...epic fail! 😫

So, maybe @jdcauley is right, until the generator is not ready.

@retlehs

This comment has been minimized.

Copy link
Member

retlehs commented Apr 15, 2015

we don't need to make decisions about how people are going to be doing their header navigation as far as collapsing and toggles go

@kalenjohnson

This comment has been minimized.

Copy link
Member

kalenjohnson commented Apr 15, 2015

Originally when this was posted, I was against it. I tend to use the Bootstrap navigation on most sites, as well as dropdowns. However, I think this opens up some interesting doors, and not even talking about the generator.

I don't think there's anything necessarily wrong with keeping this slim. Plus, if we start incorporating Composer for extra dependencies, like a Bootstrap Walker, this could really open the way for all other frameworks. It'd be pretty easy to convert that to a Foundation Walker and then include that much easier as well.

I know that a bit of the surrounding HTML will need to be added per project, unless the generator gets that sorted out. But for the time being, I think a gist will substitute nicely. It really won't take long to add it in if you're using Bootstrap or another framework.

So.... +1 for the PR

@JulienMelissas

This comment has been minimized.

Copy link
Member

JulienMelissas commented Apr 15, 2015

Ok, yea I'm using Soil, common Ben ;)

I think that as long as there are good resources for people who are used to the Bootstrap nav, I'm fine with this change. In fact, any movement towards a cleaner, leaner Sage (that still helps), I'm all for.

@AdamWills

This comment has been minimized.

Copy link

AdamWills commented Apr 15, 2015

One of the things that I enjoy with Sage is that the navigation is ready to go and with rapid prototyping, not having to worry about setting up navigation is pretty beneficial.

I'm all for making Sage framework agnostic, so this makes total sense - but would love that there can be some way to easily be able to grab the walker to continue being able to quickly get a responsive navigation component working.

@retlehs

This comment has been minimized.

Copy link
Member

retlehs commented Apr 16, 2015

the collapsible navigation in the bootstrap navbar is one of many possible responsive menu patterns. i don't think we should force anyone to use a specific pattern off the bat.

the walker change in this PR wouldn't affect you using the bootstrap navbar markup with collapse button, all you'd need to do is update the header template to have the necessary markup.

@Foxaii

This comment has been minimized.

Copy link
Member

Foxaii commented Apr 16, 2015

I'm all for the change but only after either the generator is in place or we have created step-by-step documentation to get the nav up and running using 3rd party repos. It's not arduous to add the nav back in, but adding stuff is almost always more work than deleting stuff and walkers are something that I think a decent number of WordPress devs still struggle to get their head around.

If we do lose the code for the nav then I think we should seriously consider dropping Bootstrap entirely now, in favour of becoming framework agnostic with documentation explaining how to add your framework of choice.

I appreciate that this is going from one extreme to the other, but I don't see much point in half measures.

@schrapel

This comment has been minimized.

Copy link
Contributor

schrapel commented Apr 17, 2015

I agree with @Foxaii. There is nothing else left that is Bootstrap specific (that I can remember).

@schrapel

This comment has been minimized.

Copy link
Contributor

schrapel commented Apr 17, 2015

Also, there is no need to output <nav> if there is no primary navigation.

@JulienMelissas

This comment has been minimized.

Copy link
Member

JulienMelissas commented Apr 24, 2015

Also agree with @Foxaii here on removing Bootstrap if we decide to get rid of this, as long is there is some documentation on adding BS (and/or other frameworks) back in. Yay.

@retlehs

This comment has been minimized.

Copy link
Member

retlehs commented Apr 24, 2015

the nav walker in sage right now is plugin territory, imo. bootstrap or not.

@retlehs

This comment has been minimized.

Copy link
Member

retlehs commented Apr 29, 2015

this is getting merged. if you want the old header with dropdown support, please see this gist:

https://gist.github.com/retlehs/1b49f6c00d5140962d56

i encourage someone to create a fork of https://github.com/twittem/wp-bootstrap-navwalker and maintain it, since that repo seems like it needs some love. it would be cool if it had composer support, too.


if you want to use the 'clean walker', just make sure you have soil installed and activated on your site. bootstrap navs work fine with just the clean walker as long as you don't need dropdown support.

@retlehs retlehs force-pushed the move-walker-and-remove-bootstrap-header branch from 7ea39f8 to 64e78cf Apr 29, 2015

retlehs added a commit that referenced this pull request Apr 29, 2015

Merge pull request #1427 from roots/move-walker-and-remove-bootstrap-…
…header

Remove nav walker and Bootstrap navbar

@retlehs retlehs merged commit ca0f1c2 into master Apr 29, 2015

0 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/travis-ci/push The Travis CI build is in progress
Details

@retlehs retlehs deleted the move-walker-and-remove-bootstrap-header branch Apr 29, 2015

@retlehs

This comment has been minimized.

Copy link
Member

retlehs commented May 8, 2015

for anyone coming across this that wants the old functionality:

How to restore the Bootstrap nav [walkthrough] by @smutek

masoninthesis added a commit to masoninthesis/seaside-new that referenced this pull request May 20, 2015

Revert "Merge pull request roots#1427 from roots/move-walker-and-remo…
…ve-bootstrap-header"

This reverts commit ca0f1c2, reversing
changes made to 9efeb67.

@Foxaii Foxaii referenced this pull request May 26, 2015

Closed

nav_walker? #1475

@leoquijano

This comment has been minimized.

Copy link

leoquijano commented Jun 10, 2015

Hi,

I came across this pull request after extensively searching why the nav menu with bootstrap 3 & dropdown support was removed.

I think there's no such thing as "Bootstrap menu support without dropdown", as anyone familiar with Bootstrap knows there are dropdowns, and if WordPress supports them it's logical for them to think they should be available. At least I was very confused by this change and I'm no newbie at WP + Bootstrap development.

I suggest these work-around instructions are added to the main Sage website. I feel that it if advertises Bootstrap-based themes then it shouldn't be framework-agnostic. Or backwards: if it's going to be framework-agnostic, then it shouldn't advertise being based on Bootstrap.

These are my 2 cents as an avid PHP & WordPress developer. I use Bootstrap and like their dropdowns for simple projects (as, being simple, it's really a bad thing spending too much time on them for things that can be avoided").

Maybe a fully Bootstrap compatible fork of Sage is in order? Or perhaps just a section on the website explaining how to use all of its features.

@retlehs

This comment has been minimized.

Copy link
Member

retlehs commented Jun 11, 2015

by that logic, sage should support every BS component out of the box.

sage is a starter theme and is definitely bootstrap based out of the box. we shouldn't need to update our website to mention that the nav support has changed.

this theme goes through changes all of the time, and if you're using it you should be keeping track of these changes.

your "extensive search" could have been just looking at the changelog which would have brought you here.

i understand where you are coming from and many others also share your opinion. that said, these decisions are made for a reason and if we tried to please everyone, sage would probably be a big pile of shit.

Sent from my iPhone

On Jun 10, 2015, at 1:29 PM, Leonardo Quijano notifications@github.com wrote:

Hi,

I came across this pull request after extensively searching why the nav menu with bootstrap 3 & dropdown support was removed.

I think there's no such thing as "Bootstrap menu support without dropdown", as anyone familiar with Bootstrap knows there are dropdowns, and if WordPress supports them it's logical for them to think they should be available. At least I was very confused by this change and I'm no newbie at WP + Bootstrap development.

I suggest these work-around instructions are added to the main Sage website. I feel that it if advertises Bootstrap-based themes then it shouldn't be framework-agnostic. Or backwards: if it's going to be framework-agnostic, then it shouldn't advertise being based on Bootstrap.

These are my 2 cents as an avid PHP & WordPress developer. I use Bootstrap and like their dropdowns for simple projects (as, being simple, it's really a bad thing spending too much time on them for things that can be avoided").

Maybe a fully Bootstrap compatible fork of Sage is in order? Or perhaps just a section on the website explaining how to use all of its features.


Reply to this email directly or view it on GitHub.

@leoquijano

This comment has been minimized.

Copy link

leoquijano commented Jun 11, 2015

That makes no sense. Forcing developers to comb through changelogs when they can see a formal documentation section that explains what happens and why is just bad business.

Every respected framework or library out there makes an effort to keep developers updated on changes, to guide people who used to use a certain functionality on how to enable it again (cleary and without looking at logs), and to support basic things like a bootstrap dropdown.

I'm offering suggestions based on what I know from years of experience in the field. These kinds of obscure decisions only drives people away, and your answer just points out clearly that you didn't get it: if I had to spend 2 hours figuring out why the new version of sage broke old functionality, then there's something really wrong with the development process.

Anyway, I'll use the starter theme if it makes sense, or migrate somewhere else if it doesn't. I was hoping a more open minded and professional approach, though.

@retlehs

This comment has been minimized.

Copy link
Member

retlehs commented Jun 11, 2015

are you trying to say that the changelog isn't the right place to check for changes? because you're wrong.

we have "years of experience" with running successful open source projects & your attitude/sense of entitlement is nothing new to us.

this isn't the right place to voice your complaints. you can use the roots discourse for that, as stated in our contributing guidelines.

Sent from my iPhone

On Jun 11, 2015, at 1:51 PM, Leonardo Quijano notifications@github.com wrote:

That makes no sense. Forcing developers to comb through changelogs when they can see a formal documentation section that explains what happens and why is just bad business.

Every respected framework or library out there makes an effort to keep developers updated on changes, to guide people who used to use a certain functionality on how to enable it again (cleary and without looking at logs), and to support basic things like a bootstrap dropdown.

I'm offering suggestions based on what I know from years of experience in the field. These kinds of obscure decisions only drives people away, and your answer just points out clearly that you didn't get it: if I had to spend 2 hours figuring out why the new version of sage broke old functionality, then there's something really wrong with the development process.

Anyway, I'll use the starter theme if it makes sense, or migrate somewhere else if it doesn't. Though I was hoping a more open minded and professional approach, though.


Reply to this email directly or view it on GitHub.

@JulienMelissas

This comment has been minimized.

Copy link
Member

JulienMelissas commented Jun 11, 2015

@leoquijano, while I value your opinion I disagree with a few points you've made above.

First of all, a changelog is just that: a place to log changes. We do make an effort to keep people involved and up to date using discourse, blog posts, and github issues, and we've also been very responsive to people on discourse asking where it went (https://discourse.roots.io/t/what-does-the-a-default-sage-install-look-like-screenshot/3975/2 - there are many others). In your time you spend searching you probably could have taken 2 minutes on our discourse making a new post or doing a search, I would have seen your post because I get notifications, and in my free time I would have quickly responded with a link.

Second, not every popular framework has beautiful changelogs like you're requesting. Let's take Bootstrap itself for example since you're experienced with using it - remember when they removed subnav menu support? Bootstrap doesn't have that in their documentation, but it's documented in a Pull Request just like we do. It's not even in their changelog for version 3.0.0. That made people upset. Boohoo, people still use Bootstrap, and tons of posts on the internet went up for how to fix it. Problem solved.

Third, most projects have growing pains. We can't always be 100% what we want to be. Right now, we're trying to move Sage towards being framework agnostic but we also understand many people still love and use Bootstrap and until we have a better solution, at least for now, we're keeping some of it's functionality. That's the decision of, what Ben said, is a pretty experienced team. Look at all of our green squares!

Fourth, while we love to hear people are using Sage to build awesome websites, you don't have to use it. Fork it, add your full Bootstrap support back and BOOM you've got your own starter theme, or use something else.

@toddsantoro

This comment has been minimized.

Copy link

toddsantoro commented Jun 11, 2015

@retlehs Ben, Thanks for developing such a great starter theme on the bleeding edge. I whole hardily agree with you. I just started a new project with Sage 3 days ago and when I saw the navigation I thouth things were busted. I search the discourse and did not find anything so I asked if anyone had a screen shot of the home page and was imediatly directed to a new install of the theme. I was also notified about the way to restore the navwalker. The response was within 30 minutes. Great project and don't waste time on these issues. Cheers!

@leoquijano

This comment has been minimized.

Copy link

leoquijano commented Jun 11, 2015

Hi Julien. Thanks for your answer. That's the kind of thoughtful reasoning I was looking for.

This was not a support request. That's the reason I didn't use the forums (I don't use them since I'm used to "RTFM" as old hackers used to say). Also, keep in mind that some people are used to combing through changelogs and pull requests. Some aren't.

It's a suggestion: put a Bootstrap section in the main website with instructions of how to work with common Bootstrap things. Maybe you will end up with even a module or subproject.

It's probably not something that takes too much time. I would gladly offer to do it if I thought it would be well received.

(** edited to say "reasoning" and not "argument" -- language barriers there)

@MikeiLL

This comment has been minimized.

Copy link

MikeiLL commented Jun 11, 2015

On Jun 11, 2015, at 3:25 PM, Leonardo Quijano notifications@github.com wrote:

Hi Julien. Thanks for your answer. That's the kind of thoughtful argument I was looking for.

This was not a support request. That's the reason I didn't use the forums (I don't use them since I'm used to "RTFM" as old hackers used to say). Also, keep in mind that some people are used to combing through changelogs. Some aren't.

It's a suggestion: put a Bootstrap section in the main website with instructions of how to work with common Bootstrap things. Maybe you will end up with even a module or subproject.

It's probably not something that takes too much time. I would gladly offer to do it if I thought it would be well received.

I would welcome it and started a discussion on discourse which I’d love for you to help move this thread to: https://discourse.roots.io/t/abstracting-away-from-bootstrap/4028


Reply to this email directly or view it on GitHub.

@QWp6t

This comment has been minimized.

Copy link
Member

QWp6t commented Jun 11, 2015

It's a suggestion: put a Bootstrap section in the main website with instructions of how to work with common Bootstrap things. Maybe you will end up with even a module or subproject.

Why would we add a section to the main website with instructions for how to work with Bootstrap when we've already made the decision to move away from Bootstrap?

You're essentially demanding that we provide documentation for something that has nothing to do with Sage, per se. A Bootstrap nav walker could be used for any WordPress theme that loads Bootstrap, If you're choosing to use Bootstrap, feel free to find a navbar implementation that works for you. It's patently absurd to tell us that it's our duty to document how integrate your favorite Bootstrap component into a WordPress theme.

@leoquijano

This comment has been minimized.

Copy link

leoquijano commented Jun 11, 2015

Why would we add a section to the main website with instructions for how to work with Bootstrap when we've already made the decision to move away from Bootstrap?

Well, because that's what your website says, that it's Bootstrap based:

"Sage is a WordPress starter theme based on HTML5 Boilerplate, gulp, Bower, and Bootstrap, that will help you make better themes."

@roots roots locked and limited conversation to collaborators Jun 11, 2015

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.