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

chore: remove instances of AnyDuringMigration in icons code #6376

Merged
merged 5 commits into from
Aug 23, 2022

Conversation

BeksOmega
Copy link
Collaborator

The basics

  • I branched from develop
  • My pull request is against develop
  • My code follows the style guide
  • I ran npm run format and npm run lint

The details

Resolves

Work on #5857

Proposed Changes

Removes instances of AnyDuringMigration and replaces them with better types where applicable.

Also makes some private things actually private.

Behavior Before Change

No change in behavior.

Behavior After Change

No change in bheavior.

Reason for Changes

Types are fun!

Test Coverage

N/A

Documentation

N/A

Additional Information

I didn't fix the Pid_ type in mutator because I still don't know how to properly type setTimeout return values.

this.bubble_ = null;
if (this.bubble_) {
this.bubble_.dispose();
this.bubble_ = null;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do you still need to set this.bubble_ to null here?

Copy link
Collaborator Author

@BeksOmega BeksOmega Aug 23, 2022

Choose a reason for hiding this comment

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

This is just disposing of the bubble, not the whole icon. So we need to make sure to drop references to the bubble so it can be disposed.

if (!this.isVisible()) {
return;
}
if (!this.bubble_) return;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why switch from isVisible() to bubble_ for the check?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So typescript can know that this.bubble_ is defined for the this.bubble_.getBubbleSize() call

@BeksOmega BeksOmega merged commit bb37d1b into google:develop Aug 23, 2022
@BeksOmega BeksOmega deleted the fix/any-icons branch October 4, 2022 18:09
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.

2 participants