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

[visible.directive] Set visible to false only when fully hidden. Fix #19 #20

Merged
merged 1 commit into from
Jul 27, 2015

Conversation

artoale
Copy link
Owner

@artoale artoale commented Jul 27, 2015

I've tried to fix #19 by checking if fully visible/hidden before setting the value.

This has two "issues" I could think of:

  • Inconsistency: the binded variable becomes "true" when it's fully visible, but stays as that untill is fully hidden. That's the desired behavior, but I'm just worried it might somehow lead to confusion
  • Impossible to "clear" when there's not enough room for completely hiding the target. This also is not necessarily an issue, and it's a drawback of our decision. (We would have it anyway, even with the previous solution, if the case the page is not big enough to hide the target even partially).

I believe those are not actual issues - but might be worth discuss it/keep track of it

@artoale
Copy link
Owner Author

artoale commented Jul 27, 2015

@flea89 pinging you in for review.

@artoale
Copy link
Owner Author

artoale commented Jul 27, 2015

Also, we should definitely add some tests for this scenarios (even simple ones that skips the body/scroller element complexity)

flea89 added a commit that referenced this pull request Jul 27, 2015
[visible.directive] Set visible to false only when fully hidden. Fix #19
@flea89 flea89 merged commit aaad465 into develop Jul 27, 2015
@flea89
Copy link
Collaborator

flea89 commented Jul 27, 2015

Nice catch. LGTM.
Rebasing on develop and merging.

I totally agree with you about the tests. The API are quite covered while the scroll functionality is not.
I reckon this might require some discussion though. I haven't found a nice way to test the internals so far, we can explores different way to do it though and see whatever works best.

@artoale artoale deleted the 19-scroll-spy-fix branch July 27, 2015 17:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants