fix(GlobalPositionStrategy): justifyContent center ignored when direction is RTL

[leibale]

In d3a3136 @crisbeto makes justify-content invert when the direction is RTL, but he forgot to to it only if the justify-content is flex-start or flex-end.

[crisbeto]

Out of context, it's hard to see the reasoning behind this check. I would switch it so the check is this._overlayRef.getConfig().direction === 'rtl' && this._justifyContent !== 'center'.

[leibale]

Why? Don't you want space-between (as an example) to work too?
You can see here the options for justify-content.

[crisbeto]

Since the overlay wrapper can only have one overlay panel at a time, I wouldn't worry too much about any of the other flex values.

[leibale]

I thought about another solution for that:

if (config.width === '100%') {
    parentStyles.justifyContent = 'flex-start';
} else if (this._overlayRef.getConfig().direction === 'rtl') {
    // In RTL the browser will invert `flex-start` and `flex-end` automatically, but we
    // don't want that because our positioning is explicitly `left` and `right`, hence
    // why we do another inversion to ensure that the overlay stays in the same position.
    // TODO: reconsider this if we add `start` and `end` methods.
    if (this._justifyContent === 'flex-start') {
        parentStyles.justifyContent = 'flex-end';
    } else if (this._justifyContent === 'flex-end') {
        parentStyles.justifyContent = 'flex-start';
    }
}

parentStyles.justifyContent = parentStyles.justifyContent || this._justifyContent;

Or maybe change the last line to:

if (config.width === '100%') {
    parentStyles.justifyContent = 'flex-start';
} else if (this._overlayRef.getConfig().direction === 'rtl') {
    // In RTL the browser will invert `flex-start` and `flex-end` automatically, but we
    // don't want that because our positioning is explicitly `left` and `right`, hence
    // why we do another inversion to ensure that the overlay stays in the same position.
    // TODO: reconsider this if we add `start` and `end` methods.
    if (this._justifyContent === 'flex-start') {
        parentStyles.justifyContent = 'flex-end';
    } else if (this._justifyContent === 'flex-end') {
        parentStyles.justifyContent = 'flex-start';
    }
}

if (!parentStyles.justifyContent) {
    parentStyles.justifyContent = this._justifyContent;
}

What do you think?

[crisbeto]

The latter option looks good as well.

[crisbeto]

@leibale looks like one of tests is failing.

[leibale]

@crisbeto this is because of the change I suggested in the commit comments and you accepted..
What should I do instead? Revert the code to the first commit?

[crisbeto]

Can you add another else if (this._justifyContent === 'center') in there?

[leibale]

@crisbeto this makes other tests to fail (like "GlobalPositonStrategy should overwrite previously applied positioning"), I'm reverting the code to baf4fec.

[crisbeto]

Here's what I meant. This passes all tests, including the one introduced in this PR:

    if (config.width === '100%') {
      parentStyles.justifyContent = 'flex-start';
    } else if (this._justifyContent === 'center') {
      parentStyles.justifyContent = 'center';
    } else if (this._overlayRef.getConfig().direction === 'rtl') {
      // In RTL the browser will invert `flex-start` and `flex-end` automatically, but we
      // don't want that because our positioning is explicitly `left` and `right`, hence
      // why we do another inversion to ensure that the overlay stays in the same position.
      // TODO: reconsider this if we add `start` and `end` methods.
      if (this._justifyContent === 'flex-start') {
        parentStyles.justifyContent = 'flex-end';
      } else if (this._justifyContent === 'flex-end') {
        parentStyles.justifyContent = 'flex-start';
      }
    } else {
      parentStyles.justifyContent = this._justifyContent;
    }

[crisbeto]

LGTM

[crisbeto]

nit: the indentation here is too much.

[josephperrott]

Fixes #11912

[angular-automatic-lock-bot]

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}