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

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
4 participants
@vjrantal

vjrantal commented Jan 5, 2015

Closes #2861

@vjrantal

This comment has been minimized.

Show comment
Hide comment
@vjrantal

vjrantal 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 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

This comment has been minimized.

Show comment
Hide comment
@vjrantal

vjrantal 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.

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

This comment has been minimized.

Show comment
Hide comment
@dannyg

dannyg Mar 12, 2015

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

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

This comment has been minimized.

Show comment
Hide comment
@perrygovier

perrygovier Mar 24, 2015

Member

patched in 893fcbe
Thanks!

Member

perrygovier commented Mar 24, 2015

patched in 893fcbe
Thanks!

@perrygovier perrygovier removed the ready label Mar 24, 2015

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