Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

update dependencies Grunt and Bootstrap - set active class… #2

Merged
merged 3 commits into from
May 9, 2017

Conversation

atelierbram
Copy link
Contributor

In this PR I updated the dependencies for Grunt, and others as well in package.json because after NPM first install running the grunt command Grunt wouldn't run and generates an error. Bootstraps navbar needed some additional (updated) markup too with the new version.

I opted for setting an active class on the nav's list-items (like in Bootstrap) ISO on the link in the templates ISO in javaScript. Using the comparison operator == here within an if-statement. This works when the values of the two variables page_id (in this example from html/templates/page1.html) and menu_item from the for-loop in html/components/nav.njk match, ... and because of the way these templates render:

Because assignments outside of blocks in child templates are global and executed before the layout template is evaluated it’s possible to define the active menu item in the child template:

In html/templates/page1.html:

{% set page_id = "Page 1" %}   

In html/components/nav.njk:

  <li class="nav-item {% if page_id == item.menu_item %}active{% endif %}">

package.json Outdated
},
"repository": {
"type": "git",
"url": "https://github.com/morganfeeney/bootstrap-patterns"
"url": "git+https://github.com/morganfeeney/bootstrap-patterns.git"
Copy link
Owner

@morganfeeney morganfeeney May 2, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't seen this before +git, what does it do?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry I don't know, copy & paste thing from other repo. I will remove it in a bit.

Copy link
Owner

@morganfeeney morganfeeney left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, i've spent a bit of time looking at the changes, I spotted a few things, if you don't have time just let me know and I'll pick them up ;)

There are a few outdated utility spacing classes used in the header that need to be updated, see example:
image

You can see the same alignment issue in the footer too.

I notice in the nav, the word navbar is out of alignment with the content:

image

Any idea why?

@@ -3,12 +3,12 @@
<script type="text/javascript" src="../node_modules/bootstrap/dist/js/bootstrap.js"></script>
<script src="http://localhost:35729/livereload.js"></script>
<script type="text/javascript">
var url = window.location;
// var url = window.location;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see the point to this, I originally wanted some dynamic way to do this which is why I opted for the JS at that time. For example if the name of the file changes then you might have to also change the page id etc.

However... from a learning point of view I think it's far better to show people how to add an active class using Nunjucks 👍.

@atelierbram You can go ahead and remove the JS rather than comment it out, ok?

@atelierbram
Copy link
Contributor Author

atelierbram commented May 8, 2017 via email

@morganfeeney
Copy link
Owner

I'm gonna merge this in, and fix the classes myself. If BS updates then... well it's not exactly a big job to correct. Thanks for the effort.

@morganfeeney morganfeeney merged commit 9e42e8c into morganfeeney:develop May 9, 2017
@atelierbram
Copy link
Contributor Author

Great - thanks; you're welcome

@atelierbram atelierbram deleted the update-dependencies branch May 9, 2017 08:27
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants