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: Disallow and ignore x and y attributes for blocks in toolbox definitions. #8785

Merged
merged 2 commits into from
Feb 27, 2025

Conversation

gonfunko
Copy link
Contributor

@gonfunko gonfunko commented Feb 24, 2025

The basics

The details

Resolves

Fixes #6280

Proposed Changes

This PR removes x and y from BlockInfo, and removes them if set before loading and laying out blocks in a flyout. Previously, these would cause blocks to be positioned incorrectly in the flyout, since the flyout repositions its contents relative to one another but assumes their initial origin is (0, 0). The XML codepath was not subject to this issue.

@gonfunko gonfunko requested a review from a team as a code owner February 24, 2025 23:53
@gonfunko gonfunko requested a review from BenHenning February 24, 2025 23:53
@gonfunko gonfunko changed the title fix: Disallow and ignore x and y attributes for blocks in toolbox def… fix: Disallow and ignore x and y attributes for blocks in toolbox definitions. Feb 24, 2025
@github-actions github-actions bot added PR: fix Fixes a bug and removed PR: fix Fixes a bug labels Feb 24, 2025
Copy link
Contributor

@BenHenning BenHenning left a comment

Choose a reason for hiding this comment

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

Thanks @gonfunko! Just had a couple of comments looking for clarity.

Separately, can you confirm that the actual positioning logic no longer needs to be updated per #6280? It seems that it was fixed in #7333 but just wanted to double check.

@BenHenning BenHenning assigned gonfunko and unassigned BenHenning Feb 25, 2025
Copy link
Contributor Author

@gonfunko gonfunko left a comment

Choose a reason for hiding this comment

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

Confirmed the positioning logic needs no change after this; it assumes items are initially at (0, 0) and moves them relative to that point, which was problematic when they were starting at not-(0, 0) due to the x and y properties being respected, but is no longer an issue.

@gonfunko gonfunko assigned BenHenning and unassigned gonfunko Feb 26, 2025
@BenHenning
Copy link
Contributor

Thanks @gonfunko. Just had one suggestion on clarifying a comment for clearer context, otherwise the PR LGTM.

@BenHenning BenHenning assigned gonfunko and unassigned BenHenning Feb 26, 2025
@gonfunko gonfunko assigned BenHenning and unassigned gonfunko Feb 26, 2025
@BenHenning BenHenning assigned gonfunko and unassigned BenHenning Feb 27, 2025
@gonfunko gonfunko merged commit 0ed6c82 into google:rc/v12.0.0 Feb 27, 2025
7 checks passed
@gonfunko gonfunko deleted the flyout-positioning branch February 27, 2025 18:55
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.

2 participants