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: make renderer methods public or protected #6887

Merged
merged 4 commits into from
Mar 9, 2023

Conversation

maribethb
Copy link
Contributor

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

Proposed Changes

  • Makes methods and classes related to Renderers more public.
    • If a method was only used by its subclasses, I made it protected
    • If a method was used/called/overridden by any renderer, I made it public
    • This means very few methods remained @internal, mostly only the top-level render API which is called by blocks, and the debug methods which are due for removal anyway.
  • Removes the extra documentation at the top of each file. As this comment was attached to the import of goog, it was not being surfaced in the reference docs, and was usually redundant with the real class documentation. In cases where it wasn't, I updated the real class documentation.

Behavior Before Change

No reference documentation for renderers, and mixed messages about whether we wanted external developers to subclass renderers.

Behavior After Change

Developers can subclass any renderer and read documentation about the methods they're subclassing.

Reason for Changes

Developers are allowed to subclass renderers.
As an aside, we are not entirely sure why all of these methods were @internal to begin with. The @internal annotation means it used to be @package in the closure JS days, but we never intended to block developers from subclassing renderers. The leading theory is we locked them down while the new rendering system was in development/testing/beta, and they were never unlocked afterward.

Also, this change is necessary if we were to move renderers out of core and into plugins.

Test Coverage

tsserver recognizes Blockly.blockRendering.BottomRow and Blockly.zelos.ConstantProvider (this was also true before but it's still true)

tested renderers in advanced playground

Documentation

Reference docs should now be generated.

Additional Information

@maribethb maribethb requested a review from a team as a code owner March 8, 2023 23:21
@maribethb maribethb requested a review from gonfunko March 8, 2023 23:21
@github-actions github-actions bot added the PR: feature Adds a feature label Mar 8, 2023
@rachel-fenichel rachel-fenichel self-requested a review March 9, 2023 00:11
@rachel-fenichel rachel-fenichel self-assigned this Mar 9, 2023
Copy link
Collaborator

@rachel-fenichel rachel-fenichel left a comment

Choose a reason for hiding this comment

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

LGTM after one nit.

core/renderers/geras/renderer.ts Outdated Show resolved Hide resolved
@maribethb maribethb enabled auto-merge (squash) March 9, 2023 00:29
@maribethb maribethb merged commit 8173d13 into google:develop Mar 9, 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.

Renderer visibility is too restricted
3 participants