X Tutup
Skip to content

fix(browser_adapter.ts): baseElement.getAttribute#3214

Closed
PatrickJS wants to merge 1 commit intoangular:masterfrom
PatrickJS:patch-1
Closed

fix(browser_adapter.ts): baseElement.getAttribute#3214
PatrickJS wants to merge 1 commit intoangular:masterfrom
PatrickJS:patch-1

Conversation

@PatrickJS
Copy link
Copy Markdown
Contributor

currently throwing an error

Error during instantiation of LocationStrategy! (RouterLink -> Router -> Location -> LocationStrategy). 
ORIGINAL ERROR: TypeError: baseElement.attr is not a function

closes #3441

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hey, could you add a unit test for this?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I added tests but ran into a problem with baseElement cache

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could you add a resetBaseElement method to the DomAdapter (and all implementations). You can narrow it down to only the JS/Dart adapter via if (DOM.supportsDOMEvents()) {...}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think I did something wrong here when adding resetBaseElement and getBaseHref to the other adapters

@tbosch tbosch added the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Jul 24, 2015
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

this fails due to baseElement being cache from the prevous test

@PatrickJS
Copy link
Copy Markdown
Contributor Author

I'm having trouble correctly testing this in Dart. Can anyone help?

@btford
Copy link
Copy Markdown
Contributor

btford commented Aug 3, 2015

@gdi2290 sure– what's the problem?

@btford btford self-assigned this Aug 3, 2015
@btford btford added this to the alpha-34 milestone Aug 3, 2015
@PatrickJS PatrickJS force-pushed the patch-1 branch 2 times, most recently from f32a1ad to e4379ec Compare August 3, 2015 21:04
@PatrickJS
Copy link
Copy Markdown
Contributor Author

@btford with Dart I'm trying not run these tests for Dart (since it's only in js atm). Using if (DOM.supportsDOMEvents) travis dart error doesn't seem to work when avoiding the Dart version. I recall an environment variable (IS_DARTIUM) that is used to narrow down a test to js so should I use that rather than supportsDOMEvents as Tobias suggested? Unless he meant for me to write out getBaseHref for Dart too (I know he is on vaca this week)

@btford
Copy link
Copy Markdown
Contributor

btford commented Aug 4, 2015

Unless he meant for me to write out getBaseHref for Dart too (I know he is on vaca this week)

I think that's what he meant, but I'm not sure. :/

@btford btford modified the milestones: alpha-35, alpha-34 Aug 5, 2015
@btford
Copy link
Copy Markdown
Contributor

btford commented Aug 5, 2015

Moving this to alpha-35.

currently throwing an error
```error
Error during instantiation of LocationStrategy! (RouterLink -> Router
-> Location -> LocationStrategy).
ORIGINAL ERROR: TypeError: baseElement.attr is not a function
```
@PatrickJS
Copy link
Copy Markdown
Contributor Author

@btford can you take over this PR? you can either close and recreate it or push to my repo git push patrickjs yourbranch:patch-1 for it to show up in this PR. All that's left is Dart support and I don't want this PR to live through another version :/

@btford
Copy link
Copy Markdown
Contributor

btford commented Aug 18, 2015

Yes, I'll get this in today.

@btford
Copy link
Copy Markdown
Contributor

btford commented Aug 18, 2015

Fixed and pushed to presubmit. 👍

@btford btford closed this in 235dec2 Aug 18, 2015
@PatrickJS PatrickJS deleted the patch-1 branch August 18, 2015 19:18
@PatrickJS
Copy link
Copy Markdown
Contributor Author

thanks! 👍

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews cla: yes effort2: days type: bug/fix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

HTML5LocationStrategy doesn't work (even with base attr)

4 participants

X Tutup