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

feat(overlay): overlay rtl support #1593

Merged
merged 1 commit into from
Oct 25, 2016
Merged

feat(overlay): overlay rtl support #1593

merged 1 commit into from
Oct 25, 2016

Conversation

kara
Copy link
Contributor

@kara kara commented Oct 25, 2016

Blocked on #1591 (needs rebase). Adds RTL support to overlay ref and position strategy. APIs need discussion.

r: @jelbourn

@kara kara added pr: needs review needs: discussion Further discussion with the team is needed before proceeding labels Oct 25, 2016
@googlebot googlebot added cla: yes PR author has agreed to Google's Contributor License Agreement labels Oct 25, 2016
@kara kara added the blocked This issue is blocked by some external factor, such as a prerequisite PR label Oct 25, 2016
@kara kara removed blocked This issue is blocked by some external factor, such as a prerequisite PR needs: discussion Further discussion with the team is needed before proceeding labels Oct 25, 2016
this._templatePortal = new TemplatePortal(templateRef, viewContainerRef);
}

get overlayRef(): OverlayRef {
return this._overlayRef;
}

get dir(): 'ltr' | 'rtl' {
Copy link
Member

Choose a reason for hiding this comment

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

Type is LayoutDirection from dir.ts\

Do you need this getter at all, though, instead of just saying this._dir.value? It defaults to ltr in Dir itself.

Copy link
Contributor Author

@kara kara Oct 25, 2016

Choose a reason for hiding this comment

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

Ah, didn't know about that type. Handy. I think I still need the getter in case someone isn't using dir at all (to keep it optional)

@@ -21,6 +21,9 @@ export class OverlayState {
/** The height of the overlay panel. If a number is provided, pixel units are assumed. **/
height: number | string;

/** The direction of the text in the overlay panel. */
direction: 'ltr' | 'rtl' = 'ltr';
Copy link
Member

Choose a reason for hiding this comment

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

Same here for LayoutDirection

@@ -59,6 +60,11 @@ export class OverlayRef implements PortalHost {
}
}

/** Updates the text direction of the overlay panel. **/
private updateDirection() {
this._pane.style.direction = this._state.direction;
Copy link
Member

Choose a reason for hiding this comment

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

Instead of setting the css direction property, it should probably use do

this._pane.setAttribute('dir', this._state.direction);

@jelbourn
Copy link
Member

LGTM

@kara kara added the action: merge The PR is ready for merge by the caretaker label Oct 25, 2016
@kara kara merged commit b56f520 into angular:master Oct 25, 2016
@angular-automatic-lock-bot
Copy link

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.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker cla: yes PR author has agreed to Google's Contributor License Agreement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants