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

Refactor #12031 - Component: Responsive Overlay #12096

Merged
merged 4 commits into from
Oct 26, 2022
Merged

Conversation

mertsincan
Copy link
Member

Refactor #12031

@mertsincan mertsincan added the Status: Pending Review Issue or pull request is being reviewed by Core Team label Oct 25, 2022
}

ngAfterViewChecked() {
if (!this.initialized) {
this.ngAfterViewInit();
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi, this looks weird. I think it's better to create a private method and call it instead of using the component's lifecycle method.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hi, thanks a lot for your review! I agree with you. I'll update this line.

export interface OverlayOptions {
style?: any;
styleClass?: string;
appendTo?: 'body' | HTMLElement | undefined;
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi, I think you should also add 'self' for type

 appendTo?: 'body' | 'self' | HTMLElement;

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, this enhancement already exists in our roadmap. We will port a structure like in PrimeReact. https://www.primefaces.org/primereact/setup/


export interface OverlayOnShowEvent {
container?: HTMLElement | undefined;
target?: HTMLElement | undefined;
Copy link
Contributor

Choose a reason for hiding this comment

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

It's not the same?

target?: HTMLElement;

Copy link
Member Author

Choose a reason for hiding this comment

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

No, the container is an 'overlay' element like dropdown's overlay. The target is an element that manages the overlay.
Exp;

container: div.p-overlay.p-component.ng-star-inserted....

target: div.p-dropdown.p-component.p-dropdown-open.p-dropdown-clearable...

The container is an HTML element with a 'p-overlay'(mode: 'overlay') or 'p-overlay-modal'(mode: 'modal') class depending on the mode.

Copy link
Member Author

Choose a reason for hiding this comment

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

Note: This line has been updated.

@mertsincan mertsincan merged commit cf65bdc into master Oct 26, 2022
@mertsincan mertsincan deleted the issue-r12031 branch October 26, 2022 12:14
@mertsincan mertsincan removed the Status: Pending Review Issue or pull request is being reviewed by Core Team label Oct 26, 2022
@paulstelzer
Copy link

Any idea when this will be released?

@mertsincan
Copy link
Member Author

@paulstelzer I'm working on virtualScroller issues now. We plan to release it next week.

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

Successfully merging this pull request may close these issues.

None yet

5 participants