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

#2367 - Ability to move items on the canvas #3286

Merged
merged 8 commits into from
Sep 13, 2023

Conversation

OlgaMazurina
Copy link
Collaborator

@OlgaMazurina OlgaMazurina commented Sep 11, 2023

How the feature works? / How did you fix the issue?

Check list

  • unit-tests written
  • e2e-tests written
  • documentation updated
  • PR name follows the pattern #1234 – issue name
  • branch name doesn't contain '#'
  • PR is linked with the issue
  • base branch (master or release/xx) is correct
  • task status changed to "Code review"
  • reviewers are notified about the pull request

@OlgaMazurina OlgaMazurina linked an issue Sep 11, 2023 that may be closed by this pull request
@OlgaMazurina OlgaMazurina marked this pull request as ready for review September 12, 2023 07:44
@OlgaMazurina OlgaMazurina changed the base branch from master to 2370-erase-tool September 12, 2023 07:52
await bondTwoMonomers(page, peptide3, peptide2);
await bondTwoMonomers(page, peptide3, peptide4);

await page.screenshot({
Copy link
Collaborator

Choose a reason for hiding this comment

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

We have function takeEditorScreenshot(page) which takes screenshots of result structure, name it according to test function name and place into folder - you may reuse it here and on 117 line

Copy link
Collaborator Author

@OlgaMazurina OlgaMazurina Sep 12, 2023

Choose a reason for hiding this comment

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

I decided to use this function here because I saw screenshots folder and I wanted to put it into the same place.
I replaced the way to do screenshots. Now I use takeEditorScreenshot

@@ -278,6 +279,13 @@ export abstract class BaseMonomerRenderer extends BaseRenderer {
}
}

public moveSelection(offset: Vec2) {
assert(this.rootElement);
this.monomer.moveRelative(offset);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please move this row to DrawingEntitiesManager. Renderers should not change the model. We created DrawingEntitiesManager for this purpose.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point!

@@ -30,6 +30,13 @@ export class PolymerBondRenderer extends BaseRenderer {
return this.rootBBox?.height || 0;
}

public moveSelection() {
assert(this.rootElement);
this.polymerBond.moveToLinkedMonomers();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed

Comment on lines 131 to 139
this.polymerBonds.forEach((drawingEntity) => {
command.merge(
this.createDrawingEntityMovingCommand(drawingEntity, offset),
);
});
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please mode only selected bonds and those which are connected to selected monomers

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Now it it is better, thank you!

@rrodionov91 rrodionov91 force-pushed the 2370-erase-tool branch 2 times, most recently from 0716c44 to 19b56df Compare September 13, 2023 10:59
Base automatically changed from 2370-erase-tool to master September 13, 2023 11:42
@rrodionov91 rrodionov91 force-pushed the 2367-Ability-to-move-items-on-the-canvas branch from d8c9da2 to 8059c77 Compare September 13, 2023 11:49
@rrodionov91 rrodionov91 merged commit a553367 into master Sep 13, 2023
3 of 4 checks passed
@rrodionov91 rrodionov91 deleted the 2367-Ability-to-move-items-on-the-canvas branch September 13, 2023 12:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ability to move items on the canvas
3 participants