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

fix(popover): fix popover position on Internet Explorer #2867

Closed
wants to merge 1 commit into from

Conversation

@vjrantal
Copy link

@vjrantal vjrantal commented Jan 5, 2015

Closes #2861

@vjrantal
Copy link
Author

@vjrantal vjrantal commented Jan 5, 2015

To test for regressions, I run the E2E tests on desktop Chrome with gulp snapshot. The results were the same with and without this change. Here are the popover-related results:

$ionicPopover-popover
  should init - pass
  should open left side ios popover - pass
  should close ios popover when clicking backdrop - fail
  should open middle ios popover - pass
  should open right ios popover - fail
  should open left side android popover - fail
  should close android popover when clicking backdrop - fail
  should open middle android popover - pass
  should open right android popover - fail

So some failures, but as said, same result before and after the contents of this pull request.

gulp karma passes on my environment also with this pull request's content:

Chrome 39.0.2171 (Mac OS X 10.9.5): Executed 864 of 864 SUCCESS (6.959 secs / 6.869 secs)

By the way, to run the snapshot tests locally, I had to do two edits:

  1. I noticed was that commit 7d000de changed the location to which demos ended up being generated so had to revert that for the test runner to find the demos.
  2. The tests didn't execute if the IonicReporter was enabled in the configs like it was by default. As a workaround, I commented out the reporter from config/lib/ionic-snapshot.js line 138.
@vjrantal
Copy link
Author

@vjrantal vjrantal commented Feb 25, 2015

I'd like to check is this pull request ready to go in? The issue this fixes ( #2861 ) seems to have gotten the ready label, but I am not sure if that means anything.

@dannyg
Copy link

@dannyg dannyg commented Mar 12, 2015

I would like to see this pull request accepted as well, as it fixes the referenced issue.
Any update?

@perrygovier perrygovier added this to the 1.0.0-rc2 milestone Mar 24, 2015
@perrygovier
Copy link
Member

@perrygovier perrygovier commented Mar 24, 2015

patched in 893fcbe
Thanks!

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

Successfully merging this pull request may close these issues.

4 participants