Skip to content

fix: make autoclose toggleable for flyouts#7634

Merged
BeksOmega merged 10 commits into
RaspberryPiFoundation:developfrom
BeksOmega:fix/autoclose-flyouts
Nov 7, 2023
Merged

fix: make autoclose toggleable for flyouts#7634
BeksOmega merged 10 commits into
RaspberryPiFoundation:developfrom
BeksOmega:fix/autoclose-flyouts

Conversation

@BeksOmega
Copy link
Copy Markdown
Contributor

@BeksOmega BeksOmega commented Nov 6, 2023

The basics

The details

Resolves

Fixes #7506

Also RaspberryPiFoundation/blockly-samples#1922 ? Need to check.

Proposed Changes

Makes it so that autoclose on flyouts is toggleable.

Reason for Changes

People want to make the continuous toolbox autoclose. Also app inventor wants to control the flyout from an external component, so they use an autoclosing simple flyout.

Test Coverage

Manually tested steps here.

Fixed up some change detector unit tests.

Documentation

N/A

Additional Information

N/A

Makes RaspberryPiFoundation/blockly-samples#1922 possible to do further work on. Still needs fixes to refreshSelection

@BeksOmega BeksOmega requested a review from a team as a code owner November 6, 2023 23:02
@BeksOmega BeksOmega requested a review from cpcallen November 6, 2023 23:02
@github-actions github-actions Bot added PR: fix Fixes a bug and removed PR: fix Fixes a bug labels Nov 6, 2023
@BeksOmega BeksOmega marked this pull request as draft November 6, 2023 23:15
@BeksOmega BeksOmega marked this pull request as ready for review November 6, 2023 23:59
@github-actions github-actions Bot added PR: fix Fixes a bug and removed PR: fix Fixes a bug labels Nov 6, 2023
Comment thread core/flyout_base.ts
dispose() {
this.hide();
this.workspace_.getComponentManager().removeComponent(this.id);
this.targetWorkspace.getComponentManager().removeComponent(this.id);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Was this just a bug before? It does seem odd to remove it from the component list of its own workspace which we'll dispose of anyway in just a few lines.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yesh this was indeed a bug before.

Comment thread core/flyout_horizontal.ts
}
}

if (
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It makes way more sense for this logic to be in the metrics manager, so I like that change. But I'm wondering if it has the possibility to break people who depend on the current values the metrics manager returns / will they have to update their math to account or not account for the flyout now? or was the translate added by the flyout already accounted for in the metrics?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The translation gets overwritten the next time the metrics are applied to the workspace. So we were usually overwriting this update anyway.

We definitely /could/ break people that are depending on the current metrics values. But it's unlikely to do that because we're essentially adding new functionality. We have two new possible states which are categories + always open flyout and no categories + hiding flyout, and we're properly reporting those metrics.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We should definitely manually test whether this breaks the continuous flyout though. I created a testing task in the GH project and assigned it to Rachel since she's doing release stuffs.

@BeksOmega BeksOmega assigned maribethb and unassigned cpcallen Nov 7, 2023
@BeksOmega BeksOmega removed the request for review from cpcallen November 7, 2023 21:04
@BeksOmega BeksOmega merged commit d8eb7b5 into RaspberryPiFoundation:develop Nov 7, 2023
@BeksOmega BeksOmega deleted the fix/autoclose-flyouts 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.

Make autoClose in flyouts/toolboxes toggleable

3 participants