Skip to content
This repository has been archived by the owner on Jan 7, 2022. It is now read-only.

Popover timeout handlers #77

Merged
merged 24 commits into from
Feb 16, 2017
Merged

Popover timeout handlers #77

merged 24 commits into from
Feb 16, 2017

Conversation

nathansmyth
Copy link
Collaborator

Addresses #13.

Refactor .display() by extracting 3 functions
@codecov-io
Copy link

codecov-io commented Jan 20, 2017

Codecov Report

Merging #77 into master will increase coverage by 5.08%.
The diff coverage is 36.53%.

@@            Coverage Diff             @@
##           master      #77      +/-   ##
==========================================
+ Coverage   30.87%   35.95%   +5.08%     
==========================================
  Files          45       45              
  Lines        1341     1346       +5     
  Branches      316      314       -2     
==========================================
+ Hits          414      484      +70     
+ Misses        927      862      -65
Impacted Files Coverage Δ
src/components/popover/PositionHelper.js 100% <100%> (+95.34%)
src/components/popover/PopoverRegistry.js 100% <100%> (+91.66%)
src/components/popover/PopoverDirective.js 29.41% <29.78%> (+17.59%)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9ea6ece...fab894f. Read the comment docs.

$element.on('mouseenter', display);
$element.off('mouseleave', mouseOut);
Copy link
Owner

Choose a reason for hiding this comment

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

add these to $scope.$on('$destroy' ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

+1

Nathan Smyth added 5 commits January 23, 2017 14:28
Add comments to new functions

Refactor multiple if's to switch statement
Other codeclimate changes
Remove .removeGroup() from PopoverRegistry
@nathansmyth
Copy link
Collaborator Author

Now addresses #57. Still has an issue in this area.

Also cleaned up some issues. Planning to resolve more tomorrow.

@jonshaffer
Copy link
Owner

More unit tests, any e2e tests would be awesome here

@nathansmyth
Copy link
Collaborator Author

+1 for tests

I got distracted improving the code cov score.

@nathansmyth
Copy link
Collaborator Author

Got stuck on testing mouseout closing popovers in protractor. If you have any hints, let me know?

Moving to unit testing for now.

@jonshaffer
Copy link
Owner

More info on doing that here:

http://stackoverflow.com/a/28301469/6744582

@nathansmyth
Copy link
Collaborator Author

I think I'm done here. I'd like to get to 💯 but all checks are passing and I'm ready to move on and resolve these two issues.

@nathansmyth nathansmyth merged commit 7817379 into master Feb 16, 2017
@jonshaffer jonshaffer deleted the bug/popover-hover branch February 17, 2017 20:49
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants