Skip to content
This repository was archived by the owner on May 29, 2019. It is now read-only.

feat(position): implement directional positioning #3900

Closed

Conversation

wesleycho
Copy link
Contributor

  • When positioning based off position or offset, allow specification of direction

I will work on tests for this, but I am opening this up for review so that this code can be cleaned up some (I'm not a fan of all of these if conditionals here).

This will assist with #3782 with a cleaner implementation. Implementing this part is as simple as changing template/datepicker/popup.html to

<ul class="dropdown-menu"
  ng-style="{
    display: (isOpen && 'block') || 'none',
    top: position.top,
    right: position.right,
    bottom: position.bottom,
    left: position.left
  }"
  ng-keydown="keydown($event)">

And the relevant lines in the datepickerPopup directive to

      scope.$watch('isOpen', function(value) {
        if (value) {
          scope.$broadcast('datepicker.focus');
          scope.position = $position.placement(element, attrs.datepickerPlacement, appendToBody);
          if (angular.isNumber(scope.position.bottom)) {
            if (appendToBody) scope.position.bottom -= element.prop('offsetHeight');
            scope.position.bottom += 'px';
            scope.position.top = 'initial';
          } else {
            scope.position.bottom = 'initial';
            if (!appendToBody) scope.position.top += element.prop('offsetHeight');
            scope.position.top += 'px';
          }

          if (angular.isNumber(scope.position.right)) {
            scope.position.left = 'initial';
            scope.position.right += 'px';
          } else {
            scope.position.left += 'px';
            scope.position.right = 'initial';
          }

          $document.bind('click', documentClickBind);
        } else {
          $document.unbind('click', documentClickBind);
        }
      });

Here is a Plunker with those changes implemented.

- When positioning based off `position` or `offset`, allow specification of direction
@wesleycho
Copy link
Contributor Author

It might make sense to move the conditional adding and subtracting of top and bottom values in that code block example to the placement method.

It also might make sense to just move all of the CSS logic right into the method as well.

@wesleycho wesleycho modified the milestones: 0.13.1 (npm), 0.13.2 (Performance) Jul 23, 2015
@wesleycho wesleycho modified the milestones: 0.13.2 (1.4 support), 0.13.3 (Performance) Aug 2, 2015
}
} else {
newPosition.top = offset === 'offset' ? 0 : position.top;
newPosition.left = position.left;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm quite confused about this line and 92-94. What are these cases? Are they fallbacks?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

92-94 is the default case if it is a string but invalid value.

96-98 is the default case if it has no value set.

Copy link
Contributor

Choose a reason for hiding this comment

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

If it's the default case, perhaps it be handled via a isInvalid flag (or isDefaultPosition) or something, and a check below, since the code is identical, might make sense to put it together.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense - I am not happy with how I wrote this, there are too many if-else conditionals here.

@wesleycho wesleycho modified the milestones: 0.13.3 (Fixes), 0.13.4 (Performance) Aug 9, 2015
@wesleycho wesleycho removed this from the 0.14.0 (Bootstrap 3.3) milestone Oct 9, 2015
@wesleycho wesleycho modified the milestones: 0.14.3, 1.0.0 Oct 23, 2015
@icfantv
Copy link
Contributor

icfantv commented Oct 28, 2015

Closing in favor of #4762.

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

Successfully merging this pull request may close these issues.

3 participants