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.

Sign up for GitHub

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

Jump to bottom

feat(cdk): move overlay into cdk #6100

Merged
merged 1 commit into from
Aug 11, 2017
Merged

feat(cdk): move overlay into cdk #6100

merged 1 commit into from
Aug 11, 2017

Conversation

jelbourn
Copy link
Member

I also ended up incidentally changing a lot of the imports in Material to cdk directly

@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Jul 28, 2017
devversion
devversion reviewed Jul 28, 2017
View reviewed changes
Copy link
Member

@devversion devversion left a comment

Choose a reason for hiding this comment

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

How is the SCSS bundle generation approached? I don't think that the overlay.scss file will be part of the release output right now.

src/lib/core/core.ts Outdated
export {isFakeMousedownFromScreenReader} from './a11y/fake-mousedown';

export {A11yModule} from './a11y/index';
LIVE_ANNOUNCER_PROVIDER, InteractivityChecker, FocusTrap,
Copy link
Member

Choose a reason for hiding this comment

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

Those imports should be also on separate lines (as same as the others)

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

crisbeto
crisbeto approved these changes Jul 28, 2017
View reviewed changes
Copy link
Member

@crisbeto crisbeto left a comment

Choose a reason for hiding this comment

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

LGTM, a few nits.

src/cdk/overlay/public_api.ts Outdated
import {OVERLAY_CONTAINER_PROVIDER} from './overlay-container';


export {VIEWPORT_RULER_PROVIDER} from './position/viewport-ruler';
Copy link
Member

Choose a reason for hiding this comment

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

It might be a good idea to move these exports down after the NgModule. I've found myself mistaking them for import statements on a few occasions.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

src/lib/menu/menu-trigger.ts Outdated
// tslint:disable-next-line:no-unused-variable
Overlay,
OverlayRef,
OverlayState, RepositionScrollStrategy,
Copy link
Member

Choose a reason for hiding this comment

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

The RepositionScrollStrategy should be on the next line.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

src/lib/menu/menu-trigger.ts Outdated
} from '@angular/core';
import {Direction, Directionality} from '@angular/cdk/bidi';
import {isFakeMousedownFromScreenReader} from '@angular/cdk/a11y';
import {TemplatePortal, } from '@angular/cdk/portal';
Copy link
Member

Choose a reason for hiding this comment

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

Stray comma.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

src/lib/select/select.ts
PlaceholderOptions,
MD_PLACEHOLDER_GLOBAL_OPTIONS
MD_PLACEHOLDER_GLOBAL_OPTIONS,
PlaceholderOptions
Copy link
Member

Choose a reason for hiding this comment

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

Trailing comma?

src/lib/snack-bar/snack-bar.ts Outdated
OverlayState,
LiveAnnouncer,
} from '../core';
import {ComponentRef, Injectable, Injector, Optional, SkipSelf, } from '@angular/core';
Copy link
Member

Choose a reason for hiding this comment

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

Are these commas at the end intentional?

Copy link
Member Author

Choose a reason for hiding this comment

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

Nope, removed

@jelbourn jelbourn force-pushed the overlay-cdk branch 2 times, most recently from beedc53 to 15e60c1 Compare July 28, 2017 22:59
jelbourn
jelbourn commented Jul 28, 2017
View reviewed changes
Copy link
Member Author

@jelbourn jelbourn left a comment

Choose a reason for hiding this comment

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

@devversion I plan on adding a overlay-prebuilt.scss in a follow-up PR

src/lib/core/core.ts Outdated
export {isFakeMousedownFromScreenReader} from './a11y/fake-mousedown';

export {A11yModule} from './a11y/index';
LIVE_ANNOUNCER_PROVIDER, InteractivityChecker, FocusTrap,
Copy link
Member Author

Choose a reason for hiding this comment

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

Done

src/lib/menu/menu-trigger.ts Outdated
} from '@angular/core';
import {Direction, Directionality} from '@angular/cdk/bidi';
import {isFakeMousedownFromScreenReader} from '@angular/cdk/a11y';
import {TemplatePortal, } from '@angular/cdk/portal';
Copy link
Member Author

Choose a reason for hiding this comment

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

Done

src/lib/menu/menu-trigger.ts Outdated
// tslint:disable-next-line:no-unused-variable
Overlay,
OverlayRef,
OverlayState, RepositionScrollStrategy,
Copy link
Member Author

Choose a reason for hiding this comment

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

Done

src/cdk/overlay/public_api.ts Outdated
import {OVERLAY_CONTAINER_PROVIDER} from './overlay-container';


export {VIEWPORT_RULER_PROVIDER} from './position/viewport-ruler';
Copy link
Member Author

Choose a reason for hiding this comment

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

Done

src/lib/snack-bar/snack-bar.ts Outdated
OverlayState,
LiveAnnouncer,
} from '../core';
import {ComponentRef, Injectable, Injector, Optional, SkipSelf, } from '@angular/core';
Copy link
Member Author

Choose a reason for hiding this comment

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

Nope, removed

@jelbourn
Copy link
Member Author

Waiting for #6039 to be merged so I can rebase this on top of that

@jelbourn jelbourn added the action: merge The PR is ready for merge by the caretaker label Aug 9, 2017
@jelbourn
Copy link
Member Author

jelbourn commented Aug 9, 2017

@mmalerba rebased on the renaming PR and should be ready to go

@mmalerba mmalerba added pr: needs rebase and removed action: merge The PR is ready for merge by the caretaker labels Aug 10, 2017
@mmalerba
Copy link
Contributor

please rebase

@jelbourn jelbourn mentioned this pull request Aug 11, 2017
Closed
@mmalerba mmalerba added action: merge The PR is ready for merge by the caretaker and removed pr: needs rebase labels Aug 11, 2017
@mmalerba mmalerba merged commit 4d82f83 into angular:master Aug 11, 2017
@willshowell willshowell mentioned this pull request Sep 19, 2017
Closed
@jelbourn jelbourn deleted the overlay-cdk branch April 2, 2018 22:30
@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 8, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@devversion devversion devversion left review comments

@crisbeto crisbeto crisbeto approved these changes

@andrewseguin andrewseguin Awaiting requested review from andrewseguin

@josephperrott josephperrott Awaiting requested review from josephperrott

@kara kara Awaiting requested review from kara

@mmalerba mmalerba Awaiting requested review from mmalerba

@tinayuangao tinayuangao Awaiting requested review from tinayuangao

Assignees
No one assigned
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@jelbourn @mmalerba @crisbeto @devversion @googlebot