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

Bootstrap3 migration for frontend templates #1569

Closed
wants to merge 10 commits into from

Conversation

kapt
Copy link

@kapt kapt commented Nov 21, 2014

We did our best to keep the UI close to the bootstrap2 version, but there are some differences.

Some partials were shared between dashboard templates and frontend templates, so we had to duplicate them in dashboard/partials/.

  • CSS are compiled
  • tests are passing
  • please tell us if we missed something

@mbertheau
Copy link
Contributor

Thanks for that huge amount of work!

Some comments:

  • Most template files were changed to be executable, and in many files that's the only change, thus cluttering the diff
  • Consider using https://pypi.python.org/pypi/django-widget-tweaks instead of html_tags; this should also fix the Python 3 build.
  • the image_not_found.jpg symlink from sites/sandbox/public/media was changed to a normal file
  • consider leaving bootstrap 2 in bootstrap/, and putting bootstrap 3 in bootstrap3/, to reduce diff noise.
  • If possible, consider rebuilding the branch on master, with distinct commits for every step. Afaics there's 3 steps: untangle frontend and dashboard templates, add bootstrap3, migrate dashboard templates to bootstrap 3.

I know my OCD is more severe than normal people's - I'm happy to do any or all of these steps myself as well :)

@maiksprenger
Copy link
Member

@kapt, this is great work, thank you! Can't wait to merge this. Like Markus said, we're happy to pick it up from here; just let us know how you'd like to proceed.

@mbertheau
Copy link
Contributor

For reference, this is the PR for issue #1014.

@kapt
Copy link
Author

kapt commented Nov 26, 2014

@mbertheau I am a kind of a newbie with git: I don't know how to quickly rewrite old commits and I don't have more time to learn. But I fixed the 4 points you listed above.

Sorry for the messy commits.

@mbertheau
Copy link
Contributor

@kapt very nice, thank you! We'll take it from here :)

@mbertheau
Copy link
Contributor

I pushed a preliminary reorganized branch to https://github.com/django-oscar/django-oscar/commits/bootstrap3.

@kapt I didn't understand two changes:

In 24bafbe kapt renamed the bootstrap2 responsive CSS. Was this strictly necessary, was it just to make clear that this file belongs to bootstrap2, or was there another reason?
And in 9f0a020 there's a lot of changes to dashboard HTML: control-group -> form-group, adding btn-default, row-fluid -> row. They look like accidental changes to me. What am I missing?

I will later separate frontend and dashboard further by loading no CSS or javascript in the shared base.html, and instead load them in the respective layout.html.

@kapt
Copy link
Author

kapt commented Nov 26, 2014

@mbertheau,

  • 24bafbe: renaming was not necessary, as you say it was for clarity
  • 9f0a020: yes all changes to dashboard html are accidental... oops

@mbertheau
Copy link
Contributor

I updated https://github.com/django-oscar/django-oscar/commits/bootstrap3 to remove the dashboard changes and the rename. A cursory glance at frontend and dashboard found no issues. After a thorough review from someone else I'll be glad to merge :)

@codeinthehole codeinthehole added this to the 1.1 milestone Nov 27, 2014
@itbabu
Copy link
Contributor

itbabu commented Nov 27, 2014

Hi!
Great work!

I started a new ecommerce project last week... again with oscar 👍
The plan is to use bootstrap3.

Today I worked a bit with the boostrap3 branch.

I've some small suggestions:

  • @oscarpagewidth: 960px;: increase it at least to 1200px-1300px.
    The wide desktop breakpoint is 1200px in boostrap 3 .
    I think the 960px are a reminiscence of old fixed width css frameworks. No more wasted space in large desktop screens :)
  • improve products grid:
    • remove position: relative; from class product_pod;
    • change line 98 in browse.html to something like :
      <li class="col-sm-4 col-xs-12 col-md-3 col-lg-3">{% render_product product %}</li>
    • The result should be: 4 products in a row for desktop screen, 3 for laptop/tablet and one for smartphone (or col-xs-6 if you want 2 products in a row for smartphones)
  • add class img-responsive to the category image in category.html. Maybe unrelated to PR... also sorl-thumbnail should be used for this image.
  • The left sidebar "Show results for" is too small when the page is 760px wide.
    The categories overlap the adjacent area. Reducing the right indent of subcategories should fix it
    fiction oscar sandbox.

@codeinthehole
Copy link
Contributor

I'm happy for this to be merged. Two things first though:

  • We need to update the release notes for v1.1
  • We need to have a note on which versions of Less we should be using. The Less compiles ok under less v1.7.5 but not later versions (eg v2.1.1):
$ make css
# Compile CSS files from LESS
lessc --source-map --source-map-less-inline oscar/static/oscar/less/styles.less oscar/static/oscar/css/styles.css
lessc --source-map --source-map-less-inline oscar/static/oscar/less/responsive.less oscar/static/oscar/css/responsive.css
lessc --source-map --source-map-less-inline oscar/static/oscar/less/dashboard.less oscar/static/oscar/css/dashboard.css
# Compile CSS for demo site
lessc --source-map --source-map-less-inline sites/demo/static/demo/less/styles.less sites/demo/static/demo/css/styles.css
NameError: #grid > .core > .span is undefined in /Users/david/Workspace/django-oscar/sites/demo/static/demo/less/bootstrap/navbar.less on line 208, column 3:
207 .navbar-fixed-bottom .container {
208   #grid > .core > .span(@gridColumns);
209 }

@mbertheau
Copy link
Contributor

@codeinthehole we have something about the less version here: https://github.com/django-oscar/django-oscar/blob/master/docs/source/internals/contributing/development-environment.rst#writing-lesscss Do you think there's another place we should mention the less version?

I'll update the release notes.

I also addressed 8858208#commitcomment-8755258.
Thank you for the review :)

@mbertheau
Copy link
Contributor

@ITAbu thank you very much for your suggestions. I'll try them out and add them to this PR :-)

@mbertheau
Copy link
Contributor

Note to self: fix messages error tag from bootstrap2 error to bootstrap3 danger

@mbertheau
Copy link
Contributor

@ITAbu: why would you remove position: relative from .product_pod? This positions .product_price, which is absolutely positioned at bottom: 0, at the bottom of the li, circumventing the bottom padding of the article.product_pod in the li.

mbertheau pushed a commit that referenced this pull request Nov 30, 2014
This allows us to use different CSS frameworks for frontend and dashboard.

Part of #1569
mbertheau added a commit that referenced this pull request Nov 30, 2014
Things explicitly shared between frontend and dashboard, in base.html:
  * jQuery

Things referenced in both respective layout.html files so as to allow
for hassle-free implementor customization of frontend and dashboard
independently:
  * js/bootstrap/bootstrap.min.js
  * js/oscar/ui.js

Part of #1569
mbertheau pushed a commit that referenced this pull request Nov 30, 2014
This allows us to use different CSS frameworks for frontend and dashboard.

Part of #1569
mbertheau added a commit that referenced this pull request Nov 30, 2014
Things explicitly shared between frontend and dashboard, in base.html:
  * jQuery

Things referenced in both respective layout.html files so as to allow
for hassle-free implementor customization of frontend and dashboard
independently:
  * js/bootstrap/bootstrap.min.js
  * js/oscar/ui.js

Part of #1569
mbertheau pushed a commit that referenced this pull request Nov 30, 2014
mbertheau pushed a commit that referenced this pull request Nov 30, 2014
mbertheau added a commit that referenced this pull request Nov 30, 2014
The the grid gutter padding was removed with .no-padding only on the main
catalogue browse page, leaving search results, recent products on detail
view and a number of other pages that have product pod lists in them with a
double padding.

Part of #1569
mbertheau added a commit that referenced this pull request Nov 30, 2014
Bootstrap 3 recognizes a class of widths >1170px "Large devices".

Suggestion by @itbabu.

Part of #1569
mbertheau added a commit that referenced this pull request Nov 30, 2014
mbertheau added a commit that referenced this pull request Nov 30, 2014
mbertheau added a commit that referenced this pull request Nov 30, 2014
mbertheau added a commit that referenced this pull request Nov 30, 2014
mbertheau added a commit that referenced this pull request Nov 30, 2014
@codeinthehole
Copy link
Contributor

@mbertheau Sorry, didn't realise we already mentioned that about less. I did try to check but grepped the docs for lessc which didn't match.

@itbabu
Copy link
Contributor

itbabu commented Nov 30, 2014

@mbertheau whoops my bad, it's a misprint from a copy & paste.

I had a glance at the latest commits and I have a 'lastminuteonelinechange' :)

Now there are gutters around the <li> tag containing the product_pod <article> tag,
So we don't need no more extra with space around the <img>
I'd suggest to change this line to: max-with:100%

mbertheau added a commit that referenced this pull request Dec 1, 2014
mbertheau pushed a commit that referenced this pull request Dec 1, 2014
This allows us to use different CSS frameworks for frontend and dashboard.

Part of #1569
mbertheau added a commit that referenced this pull request Dec 1, 2014
Things explicitly shared between frontend and dashboard, in base.html:
  * jQuery

Things referenced in both respective layout.html files so as to allow
for hassle-free implementor customization of frontend and dashboard
independently:
  * js/bootstrap/bootstrap.min.js
  * js/oscar/ui.js

Part of #1569
mbertheau pushed a commit that referenced this pull request Dec 1, 2014
mbertheau pushed a commit that referenced this pull request Dec 1, 2014
mbertheau added a commit that referenced this pull request Dec 1, 2014
The the grid gutter padding was removed with .no-padding only on the main
catalogue browse page, leaving search results, recent products on detail
view and a number of other pages that have product pod lists in them with a
double padding.

Part of #1569
mbertheau added a commit that referenced this pull request Dec 1, 2014
Bootstrap 3 recognizes a class of widths >1170px "Large devices".

Suggestion by @itbabu.

Part of #1569
mbertheau added a commit that referenced this pull request Dec 1, 2014
mbertheau added a commit that referenced this pull request Dec 1, 2014
mbertheau added a commit that referenced this pull request Dec 1, 2014
mbertheau added a commit that referenced this pull request Dec 1, 2014
mbertheau added a commit that referenced this pull request Dec 1, 2014
@mbertheau
Copy link
Contributor

@itbabu, thanks, very reasonable, I changed as you said :)

@mbertheau
Copy link
Contributor

I merged this to master now in 94d3b39. @kapt thank you for your contribution! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants