Skip to content

Conversation

@crisbeto
Copy link
Member

Updates to the latest version of Angular and TypeScript, and resolves the various build errors.

@google-cla google-cla bot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Sep 27, 2021
Updates to the latest version of Angular and TypeScript, and resolves the various build errors.
],
exports: [
...EXPORTED_DECLARATIONS,
MatPopoverEdit,
Copy link
Member Author

Choose a reason for hiding this comment

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

These changes are a workaround for an issue with ngcc.

Copy link
Member

Choose a reason for hiding this comment

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

Is there a link/issue for this?

Copy link
Member Author

Choose a reason for hiding this comment

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

There isn't, my changes in the PR for TS 4.4 on the framework side probably didn't cover the new generated code array spreads correctly. I'm looking into it now.

Copy link
Member Author

Choose a reason for hiding this comment

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

PR to fix the compiler: angular/angular#43618.

@crisbeto crisbeto marked this pull request as ready for review September 27, 2021 18:25
@crisbeto crisbeto added P2 The issue is important to a large percentage of users, with a workaround target: major This PR is targeted for the next major release labels Sep 27, 2021
@crisbeto crisbeto added this to the 13.0.0 milestone Sep 27, 2021
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.

LGTM. just one question.

};

/** Equivalent of `ClientRect` without some of the properties we don't care about. */
type Dimensions = Omit<ClientRect, 'x' | 'y' | 'toJSON'>;
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if it would make sense to have an explicit type (with properties defined) in overlay/position instead of doing it that way. Not feeling strongly, just an idea.

Copy link
Member Author

Choose a reason for hiding this comment

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

I went with this approach since it's still less code than listing out the properties individually on an interface.

@crisbeto crisbeto added the action: merge The PR is ready for merge by the caretaker label Sep 27, 2021
if (this._handles.length) {
this._rootElementTapHighlight = rootElement.style.webkitTapHighlightColor || '';
rootElement.style.webkitTapHighlightColor = 'transparent';
const rootStyles = rootElement.style as any;
Copy link
Contributor

@zarend zarend Sep 28, 2021

Choose a reason for hiding this comment

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

I'm generally not a fan of any, but it's acceptable in this case since the type for CSSStyleDeclaration is wrong. Here are a two suggestions that are more type safe than any.

  • rootElement.style as CSSStyleDeclaration & { [key: string]: string|null|undefined }
  • rootElement.style as CSSStyleDeclaration & { webkitTapHighlightColor?: string|null}

@mmalerba mmalerba merged commit db18b40 into angular:master Sep 29, 2021
@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 Oct 30, 2021
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 P2 The issue is important to a large percentage of users, with a workaround target: major This PR is targeted for the next major release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants