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

Upgrade to Bootstrap 3 #777

Merged
merged 43 commits into from
Nov 24, 2013
Merged

Upgrade to Bootstrap 3 #777

merged 43 commits into from
Nov 24, 2013

Conversation

jerivas
Copy link
Collaborator

@jerivas jerivas commented Sep 25, 2013

Upgrade Mezzanine from Bootstrap 2.3.1 to Bootstrap 3.0.2.

This has been merged! Mezzanine is now BS3 compatible. This is still work in progress, I'm opening the ticket to track progress and receive feedback. Struck out items have been completely converted to BS3.

Current status

Major changes

  • Mezzanine now uses two main stylesheets that extend bootstrap.css. First, mezzanine.css adds all necessary tweaks to get BS3 working with the templates. A new file, bootstrap-theme.css, defines extra visual styles for the site, making it look more like BS2. If you want a vanilla BS3 experience, simply remove the latter file from base.html.
  • Many class names from BS2 have been modified or removed from BS3. Make sure to check out the migration notes.
  • Templates now use more of Bootstrap's classes/hooks instead of implementing separate style solutions, reducing the size of mezzanine.css and necessary markup. It's recommended you become familiar with all Bootstrap components to take advantage of these resources.
  • New responsive features and a mobile-first approach are in place, giving more control on how your site displays in different screen sizes.
  • The gallery now uses Magnific Popup as image viewer. This results in less markup, responsiveness and keyboard control, among others.

Known issues

  • Top menu displays dropdowns on click, not on hover. Commit da1ded9 fixes this one, turns out we don't even need javascript! The problem right now is that mobile users have no way of displaying the dropdown in the responsive nav.

Other bugs found and fixed (not directly related to Bootstrap)

  • There is no indication during the sign up process that the account will not be activated right away (assuming settings.ACCOUNTS_APPROVAL_REQUIRED = True). Commits d20ebe6 and 89d65b2 add a message to indicate the situation to the user, similar to what happens already with ACCOUNTS_VERIFICATION_REQUIRED.
  • When a user submitted form fails validation (be it by missing required fields or invalid input), only the fields get highlighted with errors, there's no indication at the top of the form on why the submission failed. Commit 323660d addresses this in the same way Cartridge does.
  • 500 and 404 error templates used to leave the meta title blank. Fixed in 655b080.

@joshcartme
Copy link
Collaborator

As far as the menus, I would vote for coming up with some way to make them activate on hover. With Bootstrap 2 there was a tweak in place in Mezzanine's css as well as some javascript in the bootstrap-extras file. I'm not sure if any of that would work with bs3, but might be somewhere to start.

@jerivas
Copy link
Collaborator Author

jerivas commented Sep 25, 2013

I found this plugin (haven't actually used it) https://github.com/CWSpear/twitter-bootstrap-hover-dropdown/blob/master/twitter-bootstrap-hover-dropdown.js. It would allow template designers to choose between click/hover by simply defining a data-hover attribute. I think it'll be the first thing to go into bootstrap-extras.js.

@stepmr
Copy link

stepmr commented Sep 30, 2013

Sweet 👍

- pagination class is applied to ul, not wrapper div
- Page info element should be a span, not an anchor, since is not
meant to be clicked.
Also remove excesive top margin in the first header of a panel.
Includes blog post list, detail, filter panel and custom styles.
- Don't leave the meta title empty.
- Present the error in a panel.
@stephenmcd
Copy link
Owner

Amazing work Ed, thanks so much.

Like Josh said we would need to have hover be the default action for expanding menus, since Mezzanine by desgin wants parent nav items to be clickable.

One other point, and it's the big elephant in the room, is that for this to be merged in, Cartridge will need to be upgraded too.

I couldn't ask anyone to do that, but if you Ed or anyone else is feeling particularly keen, please go right ahead. Otherwise it'll be me when time permits, which might not be right away.

@stepmr
Copy link

stepmr commented Oct 1, 2013

About to start work on a cartridge project now... think I'll be using bs3... so I'll try to get something together on that front.

@foxwoods369
Copy link

I second Stephen, awesome work! I'm grabbing this early to use with the site I'm currently building, so I'll let you know if I run into any issues (though so far work on my site is going fairly slowly :( )

@foxwoods369
Copy link

Actually, one thing I noticed already is that you can resize the window to a size that puts the search form on another line, which causes the title of the body of the page to be obscured by the navbar.

@daGrevis
Copy link

daGrevis commented Oct 4, 2013

Glad this is happening! :) +1

@jerivas
Copy link
Collaborator Author

jerivas commented Oct 4, 2013

@foxwoods369 Thanks for trying it out! The issue you describe is a problem even in BS2 (you can see it in the demo site). What I've done is hide the site tagline if the window is less than 1200px wide (via BS3 "visible-lg" class). Of course, if your site title is long, or you have a lot of first-level menu entries, the navbar might not be wide enough, which will wrap the search to another line and obsure some body content.

The solutions I can think of:

  • Use a regular navbar, instead of a fixed one (remove the "navbar-fixed-top" class). This would offset the body as needed (you should also remove the body padding from mezzanine.css). Of course, the navbar will no longer be with you while scrolling.
  • Dynamically calculate the body padding from the navbar height. I can only think on JS for this.

Should we go with any of the above solutions (or others)? It would be interested to know what others think.

Major refactor of the comments template, reduces duplicate markup
and uses more of Bootstrap's goodies (alerts, icons, etc).
@stephenmcd
Copy link
Owner

I've spent a bit of time going over this. Everything works more or less, which is awesome, but there are definitely some visual regressions that will still need dealing with:

  • footer nav is no longer centred
  • margins/padding around tweets is a mess
  • form fields blow out to full width of the centre pane
  • search results message misuses the "notfication" style
  • many other areas needing more breathing space with margins/padding

I'm sure there are more.

The other big issue is that bootstrap 3 itself specifically removes visual appeal, opting to strip back to a more functional toolkit. Completely understandable for what they want to do - but it means that in the current approach for upgrading Mezzanine to bootstrap 3, we'll have a less visually appealing offering "out of the box". I don't think this is a good thing and I'm undecided on how to resolve it.

We definitely want to move forward with bootstrap - what you get out of the box should be the best toolkit possible for building completely custom looking sites as quickly and easily as possible, and I think moving forward with whatever bootstrap does is the best approach. But I think a lot of people simply set up Mezzanine for their personal blog or whatever, and are fully content with the default (current version of bootstrap used) look and leave it at that - and that's one more point on the scoreboard for Mezzanine adoption. This isn't something that can be neglected.

Ideally we'd go down the wordpress path, and provide a default theme with Mezzanine itself, designed for Mezzanine, that people can use out of the box. I personally don't have the skill to do this though.

All in all I think this branch is going in the right direction, but needs more work before we can merge it.

@thoreg
Copy link

thoreg commented Oct 10, 2013

Is there an issue tracker for the issues which were found and still open?

@AntonOfTheWoods
Copy link

As a developer starting to do web again after a long break (and wanting desperately to do it with BS3), I guess my view is biased but I am a little surprised at your comment. I wouldn't have thought many people who just want to keep "the default" would use Mezzanine at all. If you aren't a coder/web designer (or don't want to bother), then I would have assumed that there are many "better" alternatives out there - WordPress (and PHP) being the elephant in the room. For me Mezzanine (and django and python) are for people who definitely want to get their hands dirty, and that is certainly why I am moving forward with it. I can't contribute anything of value for the moment though so I'll have to leave it at that :-).

@jerivas
Copy link
Collaborator Author

jerivas commented Oct 12, 2013

Thanks to all the people contributing on this one! I'll try to address Steve's comments:

  • The footer nav not being centered is the result of using BS columns instead of custom markup/styles. I'm trying to do as much as possible with BS hooks and classes, instead of adding lots of custom styles here and there. One scenario I've seen a lot while working on this is the need to show lists without their bullets. In Mezzanine's current implementation you can see a lot of rules to achieve this in mezzanine.css; in BS3 we simply give the list a "list-unstyled" class which works out of the box. However, I'll do my best to get the footer nav centered.
  • I've not worked on the twitter app just yet.
  • Form field handling has changed in BS3, now you are required to add classes to the input elements themselves, not their wrappers, to define width and other things. This can't be done because a template tag renders the actual form field. I'll tweak mezzanine.css to set a different width with modified CSS selectors. What do you suggest? 70%?
  • Full disclosure here: I've used notifications (or alerts, as BS calls them) in other places too. They are in the blog when you filter by date/author/tag etc. and in the comments when a comment gets removed or hidden. I've included them because I believe certain information should be highlighted to the user, and they provide feedback when the user performs actions (like a searching or filtering blog posts). Could you elaborate on why you believe they are inadequate? What do others think?

Now for the padding and visual appeal: this is addressed in the pull request description, including a link to a theme by BS that brings back some of the visual flair from BS2. I have to say I share @AntonOfTheWoods opinion. As someone who uses Mezzanine to build sites for clients I can say they don't stick with defaults, most of them have at least an idea of the look they want, and it's always different. For site developers, BS provides the grid, markup and style conventions, and a lot of JS goodies. BS3 is an attempt to decouple the looks from layout and functionality, and I believe we should do the same.

Of course, there are others who want to set up a site and have it looking good without any extra steps, and Mezzanine should provide for them too. I propose creating a separate file, mezzanine-theme.css, with styles that give Mezzanine a stronger visual presence. Those who don't want to use simply have to remove it from base.html.

Lastly, while we are on the topic of themes, we should open another ticket to get a nice theme for Mezzanine, giving it a less generic look. The default home page could use some spicing too. I'm thinking on running a contest, or commissioning the work to a skilled designer attached to this community or open source in general.

@joshcartme
Copy link
Collaborator

Here are some comments from initially looking at the demo site:

  • Remove the padding from the right of the search form in the navbar so that there is equal space to the left of the site title and the right of the search form
  • Center the footer menu
    • IMO it would look better if the blog pagination was floated to the right
  • Maybe add a light gray background color to the panels to differentiate them a little more (not 100% on that, what do other think?)

That's all I got, it's looking really good Eduardo, thanks for putting all the work into this!

@jerivas
Copy link
Collaborator Author

jerivas commented Oct 23, 2013

Thanks Josh, I've implemented some of the changes you suggested. I'm ok with how the pagination and panels look right now, but as you said, it would be good to hear what others have to say.

I want to draw everybody's attention to the blog (blog post list, detail, sidebar, and comments). What do you think of the new comment style (no border, use of icons in the links)? This blog post has comments, related posts, tags and everything else to mimic how a full featured post should look in production. Check it out.

@jerivas
Copy link
Collaborator Author

jerivas commented Oct 25, 2013

Also, you can take a look at 500 and 404 error pages at http://dev.jerivas.com/500 and http://dev.jerivas.com/404

@stephenmcd
Copy link
Owner

Hey Ed,

Congratulations on such a huge effort - I've gone over this again and I think it's definitely ready to go.

I'll hold off on merging this in until Cartridge is in the same state, at which point we'll do major releases of both.

PS: I've given inline editing a quick test too, and it appears to be fine (in fact the buttons look a bit better than before), so you can cross that off the list if you like.

@jerivas
Copy link
Collaborator Author

jerivas commented Oct 29, 2013

Ah, excellent! Thanks Steve and everybody else for letting me work in this one and providing testing and feedback.

The inline editor buttons aren't using the right classes AFAIK; that's what's holding me from crossing it off the list. I would also like to pitch the idea of using Bootstrap's modals for the inline editor. They are already there and would require only minimal extra CSS and JS (if any). What do you think?

@stephenmcd
Copy link
Owner

I don't think the inline editing should have any dependencies - someone might rip bootstrap out entirely.

@jerivas
Copy link
Collaborator Author

jerivas commented Oct 29, 2013

Yes, you're right.

I expect to start work on Cartridge in the coming days. Expect the new ticket soon :)

@stephenmcd
Copy link
Owner

Awesome, thanks so much again for everything!

@stephenmcd stephenmcd mentioned this pull request Oct 30, 2013
@jerivas
Copy link
Collaborator Author

jerivas commented Oct 31, 2013

Bootstrap 3.0.1 was just released. This got me thinking: how often should we update Mezzanine's BS? This release in particular includes a LOT of bugfixes and little enhancements, but I'm thinking we should wait until 3.1 to get a lot of nice goodies.

@stephenmcd
Copy link
Owner

I don't think there's a hard and fast rule other than let's try and keep up to date with the major versions at the least.

However if a patch release is made, you'd expect the upgrade to simply be a copy and paste without any major regressions, so if that's the case for a patch release then by all means let's do it.

@jerivas
Copy link
Collaborator Author

jerivas commented Oct 31, 2013

Indeed, I applied the update and haven't found any trouble.

@stephenmcd stephenmcd merged commit a093a5d into stephenmcd:master Nov 24, 2013
@jerivas
Copy link
Collaborator Author

jerivas commented Nov 25, 2013

Hey Steve. First off, thanks so much for merging in the changes and giving guidance through all the process, it is great to give something back to a community that's been so helpful and supportive.

I just wanted to know if you're going to wait for Cartridge to release the next version of Mezzanine as you stated earlier. Additionally, I want to upgrade this branch to Bootstrap 3.0.2 and convert the inline editor to BS3. If I don't find any road blockers these changes would be implemented ASAP (one week or less). I've had to hold them off because these previous weeks have been very busy, but the calender is clear for this next week.

@stephenmcd
Copy link
Owner

Ed I've actually rethought the Cartridge dependency and decided we don't need to wait for it, due to the same scenario being in place with Python 3 - I think waiting for Bootstrap 3 and Python 3 in Cartridge is too much of a blocker, so we'll just specify the current version of Mezzanine as the max version supported by Cartridge, and release the new Mezzanine - Cartridge can catch up when time permits :-)

@jerivas
Copy link
Collaborator Author

jerivas commented Nov 26, 2013

Great, seems like the right way to go. I'll catch up with master and implement the remaining small changes.

@stephenmcd
Copy link
Owner

Awesome. You might wanna glance over 9708e02 - just a bunch of tweaks I made, mostly padding/margin changes and a few other bits.

@Noahsok Noahsok mentioned this pull request Dec 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants