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 rendered blocks render themselves on construction #7355

Closed
wants to merge 5 commits into from

Conversation

BeksOmega
Copy link
Collaborator

@BeksOmega BeksOmega commented Aug 3, 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 #5146

Proposed Changes

Makes it so that BlockSvgs automatically render themselves when they are constructed.

Reason for Changes

Before this change external devs needed to call initSvg and render on their blocks if they were instantiating them via workspace.newBlock. But I deprecated render recently. This makes it so calling render is no longer necessary.

Test Coverage

Tested that loading the spaghetti code, and instantiating via newBlock both worked as expected.

Documentation

N/A

Additional Information

I decided to have these queue the render instead of rendering immediately. External devs calling render currently will trigger the immediate render, so this is non-breaking. And this way they can properly switch to using finishQueuedRenders.

@BeksOmega BeksOmega requested a review from a team as a code owner August 3, 2023 16:29
@github-actions github-actions bot added the PR: fix Fixes a bug label Aug 3, 2023
@BeksOmega BeksOmega requested review from maribethb and removed request for rachel-fenichel August 3, 2023 16:29
Copy link
Contributor

@maribethb maribethb left a comment

Choose a reason for hiding this comment

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

This change itself seems fine, but it seems like #5146 still has more work to be done but this PR says it closes it. For example, you did not remove the TODO comment on the initBlock function in serialization/blocks. Which makes sense, because in #5146 you said you want to refactor the serialization system so that it does not need to do checking based on whether the block is rendered or not. Are you still planning on addressing that? If not could you remove the todo comment and if so could you update this PR so it doesn't say it's closing that issue?

Also, there are a lot of places in core where we still call initSvg and then render / queueRender. Are you planning to update those as well? If not in this PR, could you file an issue for that if one doesn't exist?

@github-actions github-actions bot added PR: fix Fixes a bug and removed PR: fix Fixes a bug labels Aug 4, 2023
@BeksOmega
Copy link
Collaborator Author

BeksOmega commented Aug 4, 2023

This change itself seems fine, but it seems like #5146 still has more work to be done...

Ah yeah you're right, I should have marked this as "work on", not closing. I'm planning to punt on fully cleaning up initBlock because I don't fully remember why the updateDisabled or onLocationChange calls are necessary.

Also, there are a lot of places in core where we still call initSvg and then render / queueRender. Are you planning to update those as well? If not in this PR, could you file an issue for that if one doesn't exist?

Gosh darn it :P I was trying to do too many things at once yesterday. I checked for references on devsite but not other places in core haha.

So still need to:

  • Clean up references to initSvg and then queueRender in core.
  • Clean up references to initSvg and then render in other parts of this repo.
  • Clean up references to initSvg and then render in samples.

Please tell me if there's anything I'm forgetting again!

@BeksOmega BeksOmega marked this pull request as draft August 4, 2023 15:06
@BeksOmega
Copy link
Collaborator Author

This ran into problems because having the constructor call initSvg and queueRender means we can end up in a state where initSvg has been called but rendered is false. Our life cycle does not expect this to happen and it causes bugs.

So we're going to refactor rendered first and then make this change.

That means that for v10.1 I'm going to open up a different PR dedeprecating render.

@BeksOmega BeksOmega closed this Aug 4, 2023
@BeksOmega BeksOmega mentioned this pull request Aug 4, 2023
4 tasks
@BeksOmega BeksOmega mentioned this pull request Aug 28, 2023
13 tasks
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