-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
Conversation
once upon a time, the starter theme had two header templates:
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? |
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? |
I'm in favor of keeping it until the generator is there to replace it. |
roots/soil#73 is still compatible with bootstrap navigation menus because it adds the 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. |
what do you need from it - the dropdowns? (also, are you not using soil?)
what does that even mean? are you using dropdowns or what? |
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. |
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. |
I'm in favour of this cleaning up, but what about the BS collapsing navbar (and related button)? Usually I override the default header with a custom one: 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. |
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 |
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 |
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. |
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. |
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. |
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. |
I agree with @Foxaii. There is nothing else left that is Bootstrap specific (that I can remember). |
Also, there is no need to output |
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. |
the nav walker in sage right now is plugin territory, imo. bootstrap or not. |
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. |
7ea39f8
to
64e78cf
Compare
…header Remove nav walker and Bootstrap navbar
for anyone coming across this that wants the old functionality: |
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. |
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
|
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. |
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
|
@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. |
@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! |
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) |
On Jun 11, 2015, at 3:25 PM, Leonardo Quijano [email protected] wrote:
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
|
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. |
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." |
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))