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

fix: have icons use the new render management system #7296

Merged
merged 2 commits into from
Jul 24, 2023

Conversation

BeksOmega
Copy link
Collaborator

@BeksOmega BeksOmega commented Jul 17, 2023

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 #7068

Proposed Changes

  1. Adds a new method to the render management system to immediately trigger any queued renders. This should only be used in cases where it's necessary for backwards compatibility (like this one).
  2. Switches icon methods over to use the render management system (with the immediate trigger).
  3. Adds dummy promises to the return value of all setBubbleVisible methods.

Reason for Changes

  1. We need adding icons to trigger a render immediately so that if the bubbles are set to be visible immediately, they are positioned correctly. Eventually we want to move this to be promises-based (so setting the visibility will wait for any queued renders to finish first). But that is a breaking change.
  2. This gets us one step closer to tearing out the old render management system.
  3. This makes it easier for external devs to transition to the new promises based system.

Test Coverage

All tests pass! Also quickly tested block.setCommentText('test'); block.getIcon('comment').setBubbleVisible(true); in the browser console to reassure myself that it was indeed working.

@cpcallen cpcallen added the deprecation This PR deprecates an API. label Jul 24, 2023
Copy link
Contributor

@cpcallen cpcallen left a comment

Choose a reason for hiding this comment

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

Make sure that this PR still has a deprecation tag once it's been submitted: one of the bots seems to like to try to remove that tag.

Not for this PR, but: given similarity of code have you considered introducing a BubbleIcon intermediate class between Icon and Comment/Mutator/WarningIcon?

@cpcallen cpcallen changed the title fix: have icons use the new render management sytem fix: have icons use the new render management system Jul 24, 2023
@github-actions github-actions bot added PR: fix Fixes a bug and removed PR: fix Fixes a bug deprecation This PR deprecates an API. labels Jul 24, 2023
@github-actions github-actions bot added PR: fix Fixes a bug and removed PR: fix Fixes a bug labels Jul 24, 2023
@BeksOmega
Copy link
Collaborator Author

BeksOmega commented Jul 24, 2023

Maribeth actually requested on the next pull request that I remove the dummy promises, and we just cleanly break people later. So I've reverted that change and removed the deprecation notice!

[Edit: Wrt the BubbleIcon I had not considered doing that, but may be worth investigating!]

@BeksOmega BeksOmega merged commit 36ba408 into google:develop Jul 24, 2023
@BeksOmega BeksOmega deleted the fix/queue-icons branch May 14, 2024 16:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: fix Fixes a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants