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

RTL support in $position service #2811

Closed
mevjak opened this issue Oct 9, 2014 · 11 comments
Closed

RTL support in $position service #2811

mevjak opened this issue Oct 9, 2014 · 11 comments

Comments

@mevjak
Copy link

mevjak commented Oct 9, 2014

I have an issue with Popover directive on page with RTL languages. Popover horizontal position is calculated incorrectly:

popover-ltr-rtl

<a class="popover-info" popover-append-to-body="true" popover-trigger="click" popover-placement="left" popover="{{'settingsLoseProtection' | translate}}">Info</a>

Is RTL support missing in $position service? Or is it needed to make some Popover template tweaks?

Thanks for any advice.

@pkozlowski-opensource
Copy link
Member

@mevjak as of today the $position service is not aware about the LTR / RLT settings. Honestly speaking at this point I'm not sure if the fix should be done in the $position service or in the tooltip / popover itself. Would you care to investigate it further?

@mevjak
Copy link
Author

mevjak commented Oct 17, 2014

After quick research I see 2 main goals:

  1. detect LTR / RTL mode (we have 2 options):
    • either: detect current mode from DOM (not too safe in my opinion, but I'm not sure)
    • or: add new attribute to ui-bootstrap directives i.e. "popover-direction"
  2. add RTL support to $position service, especially in positionElements method - simply "mirror" position for RTL mode (see commented lines beginning with 'RTL' below):
// ui-bootstrap.js  line 907

      /**
       * Provides coordinates for the targetEl in relation to hostEl
       */
      positionElements: function (hostEl, targetEl, positionStr, appendToBody) {

        var positionStrParts = positionStr.split('-');
        var pos0 = positionStrParts[0], pos1 = positionStrParts[1] || 'center';

        var hostElPos,
          targetElWidth,
          targetElHeight,
          targetElPos;

        hostElPos = appendToBody ? this.offset(hostEl) : this.position(hostEl);

        targetElWidth = targetEl.prop('offsetWidth');
        targetElHeight = targetEl.prop('offsetHeight');

        var shiftWidth = {
          center: function () {
            return hostElPos.left + hostElPos.width / 2 - targetElWidth / 2;
          },
          left: function () {
            return hostElPos.left;
           //RTL: return hostElPos.left - hostElPos.width / 2;
          },
          right: function () {
            return hostElPos.left + hostElPos.width;
           //RTL: hostElPos.left + 1.5 * hostElPos.width;
          }
        };

        var shiftHeight = {
          center: function () {
            return hostElPos.top + hostElPos.height / 2 - targetElHeight / 2;
          },
          top: function () {
            return hostElPos.top;
          },
          bottom: function () {
            return hostElPos.top + hostElPos.height;
          }
        };

        switch (pos0) {
          case 'right':
            targetElPos = {
              top: shiftHeight[pos1](),
              left: shiftWidth[pos0]()
              //RTL: left: shiftWidth['left']() - targetElWidth
            };
            break;
          case 'left':
            targetElPos = {
              top: shiftHeight[pos1](),
              left: hostElPos.left - targetElWidth
              //RTL: left: shiftWidth['right']()
            };
            break;
          case 'bottom':
            targetElPos = {
              top: shiftHeight[pos0](),
              left: shiftWidth[pos1]()
            };
            break;
          default:
            targetElPos = {
              top: hostElPos.top - targetElHeight,
              left: shiftWidth[pos1]()
            };
            break;
        }

        return targetElPos;
      }

What's your opinion?

@pkozlowski-opensource
Copy link
Member

@mevjak IMO it would be better to detect LTR / LTR setting from the DOM, if possible. Then yes, we would need to modify the $position service but while doing so we should make sure that there is not too much code added. There is extensive code coverage so hopefully refactoring to remove any potential code duplication should be easy.

@blowsie
Copy link

blowsie commented Jul 14, 2015

I noticed a similar issue in the datepicker
image

Also the buttons are flipped.

@georgybu
Copy link

duplicate discussion: #3151

@deeg
Copy link
Contributor

deeg commented Oct 26, 2015

+1 for adding RTL support.

Is this something you guys are still considering adding in?

@icfantv
Copy link
Contributor

icfantv commented Oct 28, 2015

Closing in favor of #4762.

@icfantv icfantv closed this as completed Oct 28, 2015
@evilaliv3
Copy link

the fix of @mevjak mixed with the following would probably help:

var dir = document.documentElement.dir;
if (dir === 'rtl') {
   return x;
} else {
   return z;
}

this way depending on <html dir=rtl/ltr> all will work automagically and the dir attribrut could be changed at runtime by a possible language selector.

@evilaliv3
Copy link

evilaliv3 commented Aug 27, 2016

Any new on this ticket?

I really makes the library not usable for RTL projects :(

here you can see what happens on the up to date version of the library: globaleaks/globaleaks-whistleblowing-software#1755

@blowsie
Copy link

blowsie commented Aug 30, 2016

@evilaliv3 I used this, alongside some custom glyphicon > font-awesom less mapping (possibly one or two other fixes) and everything works a charm.

https://github.com/morteza/bootstrap-rtl

@evilaliv3
Copy link

thanks @blowsie

i'm already using https://github.com/MohammadYounes/RTL-bootstrap but the issue is in mixing it with the javascript provided by angular-ui

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

No branches or pull requests

8 participants