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

feat: add after queued renders promises to render management #6981

Merged
merged 10 commits into from
Apr 19, 2023

Conversation

BeksOmega
Copy link
Collaborator

@BeksOmega BeksOmega commented Apr 14, 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

Fixes #6978

Proposed Changes

Adds a promise to the render management system that is resolved when all queued renders have completed.

Reason for Changes

Allows extra behavior to be tacked on to the end of rendering (useful for things that need updated positioning / size information for blocks).

Test Coverage

Added tests that the promise is actually resolved.

Documentation

N/A, but related to #6980

Additional Information

We are now exporting the render management system (which only exports the afterQueuedRenders function. I exported this because (as Maribeth discovered) elements on the widget div will need to delay repositioning until after rendering. And we do allow external developers to add elements to the widget div.

Also modifies the insertion marker manager to actually use the promise. Manually verified that it works as expected.

@BeksOmega BeksOmega requested a review from a team as a code owner April 14, 2023 16:28
@BeksOmega BeksOmega requested review from NeilFraser and maribethb and removed request for NeilFraser April 14, 2023 16:28
@github-actions github-actions bot added the PR: feature Adds a feature label Apr 14, 2023
@BeksOmega BeksOmega assigned maribethb and unassigned NeilFraser Apr 14, 2023
@github-actions github-actions bot added PR: feature Adds a feature and removed PR: feature Adds a feature labels Apr 14, 2023
* @returns A promise that resolves after the currently queued renders have
* been completed.
*/
export function afterQueuedRenders(): Promise<void> {
Copy link
Contributor

Choose a reason for hiding this comment

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

overall i think this approach is more readable, thanks for updating it.

i'm still not totally sold on the name of this function but i haven't come up with anything that's better so feel free to submit but here's some ideas in case any of them conjure up anything good for you

queuedRenders
waitForQueuedRenders
rendersInProgress
doRenders
doQueuedRenders

etc. I think the reason I don't like it is after implies the work that is done "after" the current renders, but that's not what is returned by this function. The work that is done "after" the current renders is actually whatever is passed to the then that will follow this function call. So I am trying to find a name that makes this function refer to the actual queued renders themselves, while not being misleading into making you think calling this function will actually kick off a new render cycle. I don't think I've succeeded there.

So I know doRenders is already the name of a function but for example doQueuedRenders().then(doMoreWork) reads better to me than afterQueuedRenders().then(doMoreWork) because the latter makes it sound like there is some other work that is after the queued renders that we're waiting on, and I wonder how I sign up to add work to that.

So basically I'm probably overthinking it and this is approved, but I'll check one more time if you have any better naming ideas than I did...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Discussed offline and went with finishQueuedRenders.

Works pretty well as both await finishQueuedRenders() and finishQueuedRenders().then(). Also communicates that there's already some work being done, and that we are waiting until the end of it :D

@BeksOmega BeksOmega merged commit 83e3dca into google:develop Apr 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: feature Adds a feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add promise based API to render queueing
3 participants