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: removed X & Y from toolbox.ts and replaced movBy to moveTo #7333

Merged

Conversation

varshneydevansh
Copy link
Contributor

@varshneydevansh varshneydevansh commented Jul 29, 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

Proposed Changes

The BlockInfo type should be updated to not include X and Y.
Blockly has two kinds of flyouts built-in, horizontal and vertical. You should update the layout logic in both of them.
The flyouts should be changed to use moveTo instead of moveBy

Behavior Before Change

The BlockInfo type was picking up the values of x and y henceforth the position of the Toolbox Element was getting changed.

Behavior After Change

After removing the X and Y from the BlockInfo type the Toolbox Element is now ignoring the value and introducing the chnage to the code in files

to use moveTo instead of moveBy

Ignoring the x and y properties on blocks would allow them to position correctly.

Reason for Changes

Because, the blocks were using the relative coordinates instead of the absolute coordinates

Test Coverage

Browser which I used is MS Edge Version 115.0.1901.188 (Official build) (64-bit)

Documentation

Additional Information

It's my first PR on this project, so I might make mistakes.

@varshneydevansh varshneydevansh requested a review from a team as a code owner July 29, 2023 12:14
@github-actions github-actions bot added the PR: fix Fixes a bug label Jul 29, 2023
@varshneydevansh
Copy link
Contributor Author

@BeksOmega can you help me with the changes which I made are correct or not?

@BeksOmega
Copy link
Collaborator

@BeksOmega can you help me with the changes which I made are correct or not?

Hello :D Currently the review for this is assigned to @maribethb ! I'll ask her if she wants me take it =)

@BeksOmega BeksOmega assigned BeksOmega and unassigned maribethb Aug 2, 2023
@BeksOmega BeksOmega requested review from BeksOmega and removed request for maribethb August 2, 2023 16:33
Copy link
Collaborator

@BeksOmega BeksOmega left a comment

Choose a reason for hiding this comment

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

This LGTM =) Could you update the PR description to describe how you tested this? Once we have info about that and CI passes I'll merge this!

core/flyout_horizontal.ts Show resolved Hide resolved
@varshneydevansh
Copy link
Contributor Author

I will do this in the morning, tho. I did try doing the original steps earlier and found no changes. Furthermore, I will do it in the morning with a fresh perspective.

@varshneydevansh
Copy link
Contributor Author

Hi @BeksOmega ,

I tested it now and the newly modified proposed changes are working as they were expected.

I am attaching the SS for both the modified code and the unchanged code with different value in playground.html and change getToolboxElement

  • Modified code -

image

image

  • Unchanged code -

image

image

image

@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

Sweet looks great! Thank you for the fix and the testing @varshneydevansh :D

@BeksOmega BeksOmega merged commit dbe926d into google:develop Aug 4, 2023
@varshneydevansh varshneydevansh deleted the flyouts_position_incorrect_6280 branch August 4, 2023 16:19
BeksOmega added a commit that referenced this pull request Aug 10, 2023
BeksOmega added a commit that referenced this pull request Aug 10, 2023
…To (#7333)" (#7375)

This reverts commit dbe926d.

The reverted commit made it so that RTL flyouts were rendered incorrectly.
ericblackmonGoogle pushed a commit that referenced this pull request Aug 10, 2023
…To (#7333)" (#7375)

This reverts commit dbe926d.

The reverted commit made it so that RTL flyouts were rendered incorrectly.

(cherry picked from commit 7bca438)
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.

Built-in flyouts position blocks with x and y values incorrectly
3 participants