Skip to content

Conversation

RobbieTheWagner
Copy link
Member

No description provided.

<a href="{{link.href}}" class="nav-link" {{action navbar.collapse}}>{{link.name}}</a>
{{/if}}
{{#if (eq link.type 'dropdown')}}
{{#bs-dropdown tagName="li" as |dd|}}

Choose a reason for hiding this comment

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

You should use the contextual component here: nav.dropdown. This will also prevent the dropdown menu to use popper and absolute positioning.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, thanks. I missed that in the docs.

Choose a reason for hiding this comment

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

Yeah, it's missing in the examples. I just added something there: http://www.ember-bootstrap.com/#/components/navbars.

collapsed=collapsed
fluid=true
onCollapse=(action (mut collapsed) true)
onExpand=(action (mut collapsed) false)

Choose a reason for hiding this comment

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

If you don't need control over collapsed, then you can remove these two action handlers and collapsed=collapsed.

Copy link
Member Author

Choose a reason for hiding this comment

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

👍

@mansona
Copy link
Member

mansona commented Feb 14, 2018

Howdy 👋

I just tried this branch in the deprecations app (doing a bunch of experiments) but it failed with the following error:

➜  deprecation-app git:(feature/styleguide-navbar) ✗ npm start

> deprecation-app@0.0.0 start /Users/mansona/git/stonecircle/opensource/ember-guides/deprecation-app
> ember serve

Npm package "bootstrap-sass" is missing, but is typically required for SASS support. Please run `ember generate ember-bootstrap` to install the missing dependencies!
Cannot find module 'bootstrap-sass/package.json' from '/Users/mansona/git/stonecircle/opensource/ember-guides/deprecation-app'

It seems like the current implementation of bootstrap into ember-styleguide requires bootstrap to independently be added to the consuming app 🤔

And when I do that... well... things go all screwy 😂 (edit: actually because of the above process of adding ember-bootstrap to the consuming app it used bootstrap 3 by default 😖)

The general styles have still changed in the consuming app, so it's probably something to consider at this point, thoughts?

PR for adding this to the Deprecations app: ember-learn/deprecation-app#67 and the netlify build url: https://deploy-preview-67--ember-deprecation-app.netlify.com/

You can see that it has changed since the last PR buid: https://deploy-preview-62--ember-deprecation-app.netlify.com/

@simonihmig
Copy link

simonihmig commented Feb 14, 2018

It seems like the current implementation of bootstrap into ember-styleguide requires bootstrap to independently be added to the consuming app 🤔

Yes, that is a known limitation of bringing in addons as a nested dependency: their default blueprint does not get executed. In this case this is used to add a few dependencies (e.g. Bootstrap itself) directly to the consuming app's package.json, so the user is in control of updating it.

In this case ember generate ember-bootstrap --bootstrap-version=4 would have been needed.

There might be a way to work around this, by invoking the above blueprint as part of this addons own default blueprint. So the dependencies still get added to the consuming app, but automatically by ember install ember-styleguide.

This shouldn't be too hard, if you folks are interested in this, I should be able to create a PR!

@RobbieTheWagner
Copy link
Member Author

@simonihmig we don't need to pull in the deps in consuming apps, we just need the config part in ember-cli-build.js. We got it sorted. Thanks!

@MelSumner
Copy link
Member

@rwwagner90 are you okay for this to get merged and we can work on it from here?

@RobbieTheWagner RobbieTheWagner merged commit 170e947 into ember-learn:master Feb 15, 2018
@RobbieTheWagner RobbieTheWagner deleted the navbar branch February 15, 2018 19:08
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.

4 participants