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

forward arguments in <BsNavbar::LinkTo> to <BsLinkTo> #1513

Merged
merged 3 commits into from May 19, 2021

Conversation

jelhan
Copy link
Contributor

@jelhan jelhan commented May 18, 2021

Due to this bug <BsNavbar> was basically unusable. It was introduced in #1455, which has not been released yet. I have only noticed accidentally when trying to fix the docs app.

The bug uncovers a lack in our test coverage. I think we could cover at least some cases by tests if asserting for href attribute in an integration test or by using an acceptance test. Will try to investigate that one later.

I think we should add tests before merging. But feel free to merge without tests if it blocks a release otherwise.

@jelhan jelhan added the bug label May 18, 2021
@jelhan
Copy link
Contributor Author

jelhan commented May 18, 2021

Tests are failing for Ember 3.16 and 3.20 with the following error:

not ok 396 Chrome 90.0 - [85 ms] - Integration | Component | bs-navbar: Clicking expanded navbar link collapses navbar
    ---
        actual: >
            [object Object]
        stack: >
            TypeError: Cannot read property 'isActiveIntent' of undefined
                at RouterService.isActive (http://localhost:7357/assets/vendor.js:20245:27)
                at LinkComponent.get (http://localhost:7357/assets/vendor.js:107322:54)
                at http://localhost:7357/assets/vendor.js:39226:29
                at runInAutotrackingTransaction (http://localhost:7357/assets/vendor.js:56788:9)
                at track (http://localhost:7357/assets/vendor.js:57465:9)
                at LinkComponent.desc.get (http://localhost:7357/assets/vendor.js:39225:40)
                at getPossibleMandatoryProxyValue (http://localhost:7357/assets/vendor.js:15465:19)
                at _getProp (http://localhost:7357/assets/vendor.js:15530:17)
                at _get3 (http://localhost:7357/assets/vendor.js:15516:55)
                at wrapperClass.callback (http://localhost:7357/assets/vendor.js:41553:31)
        message: >
            Cannot read property 'isActiveIntent' of undefined
        negative: >
            false
        browser log: |
    ...

Seems to be an issue with router service and integration tests. It maybe related to this bug fix, which landed in Ember 3.24.

@simonihmig
Copy link
Contributor

Seems to be an issue with router service and integration tests. It maybe related to this bug fix, which landed in Ember 3.24.

I wanted to point out that I had to do this in my PR, only to find out you already figured it out! 👍

Copy link
Contributor

@simonihmig simonihmig left a comment

Choose a reason for hiding this comment

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

The bug uncovers a lack in our test coverage. I think we could cover at least some cases by tests if asserting for href attribute in an integration test or by using an acceptance test. Will try to investigate that one later.

Yeah. For navs we have this test: https://github.com/kaliber5/ember-bootstrap/blob/master/tests/acceptance/bs-nav-link-test.js
(also I just saw that it only tests .active handling, not href. Maybe we should add thishere also?)

Something like this for navbars maybe?

@simonihmig simonihmig merged commit 7964a95 into master May 19, 2021
@simonihmig simonihmig deleted the fix-argument-forwarding-in-bs-navbar-link-to branch May 19, 2021 18:15
@simonihmig
Copy link
Contributor

Merged now without a test. A follow-up PR adding the test would be great! 😉

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

Successfully merging this pull request may close these issues.

None yet

2 participants