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 getWorkspace, getFlyout, and getToolbox public #6666

Merged
merged 1 commit into from
Dec 1, 2022

Conversation

NeilFraser
Copy link
Contributor

Amusingly Flyout.isVisible and .setVisible are public, but there's no way to access a flyout given that getFlyout was not public.

Amusingly Flyout.isVisible and .setVisible are public, but there's no way to access a flyout given that getFlyout was not public.
@NeilFraser NeilFraser requested a review from a team as a code owner November 29, 2022 19:32
@BeksOmega BeksOmega removed their assignment Nov 29, 2022
@BeksOmega BeksOmega requested review from maribethb and removed request for BeksOmega November 29, 2022 20:47
@BeksOmega
Copy link
Collaborator

This LGTM but reassigned approval to Maribeth since this is a design decision. Also relevant forum link for context: https://groups.google.com/g/blockly/c/31Om9fkkAWA/m/fQuAg2-JBAAJ

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.

LGTM, but as Rachel said in the forum post, this is not just a documentation change, since we are formalizing these as part of the API. Even before TypeScript, these were marked @package so even though they have been used and recommended publicly they have never been actually declared as such.

So I would rename this PR: feat: make getWorkspace, getFlyout, and getToolbox public to make better release notes.

@cpcallen cpcallen changed the title docs: make three methods not internal feat: make getWorkspace, getFlyout, and getToolbox public Dec 1, 2022
@github-actions github-actions bot added PR: feature Adds a feature and removed PR: docs labels Dec 1, 2022
@cpcallen
Copy link
Contributor

cpcallen commented Dec 1, 2022

Because I really enjoy admiring Chesterton's Fence: do we know why these were originally @package and not @public? @rachel-fenichel?

@BeksOmega
Copy link
Collaborator

Because I really enjoy admiring Chesterton's Fence: do we know why these were originally @package and not @public? @rachel-fenichel?

I looked at the original PR (#771) and couldn't find a reason.

@NeilFraser NeilFraser merged commit 1cccc3c into develop Dec 1, 2022
@rachel-fenichel
Copy link
Collaborator

Back then we didn't really talk about the flyout as a public API. There weren't any ways to customize it.

@NeilFraser NeilFraser deleted the fraser-not-internal branch December 2, 2022 01:11
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.

None yet

5 participants