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

#2370 - erase tool for macromolecules editor #3217

Merged
merged 2 commits into from
Sep 13, 2023
Merged

Conversation

rrodionov91
Copy link
Collaborator

@rrodionov91 rrodionov91 commented Aug 30, 2023

Closes #2370
Closes #2360

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

(Screenshots, videos, or GIFs, if applicable)

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

@rrodionov91 rrodionov91 changed the title #2370 - erase tool #2370 - erase tool for macromolecules editor Aug 30, 2023
@rrodionov91 rrodionov91 force-pushed the 2370-erase-tool branch 3 times, most recently from 2a0faec to b42346a Compare August 30, 2023 15:30
await page.getByText('Tza').click();

// Create 4 peptides on canvas
await page.mouse.click(300, 300);
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should avoid using magic numbers

await page.getByText('Tza').click();

// Create 4 peptides on canvas
await page.mouse.click(300, 300);
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should avoid using magic numbers

await page.getByText('Tza').click();

// Create 4 peptides on canvas
await page.mouse.click(300, 300);
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should avoid using magic numbers

await bondTwoMonomers(page, peptide3, peptide4);

await page.screenshot({
path: 'tests/Macromolecule-editor/screenshots/erase-tool.png',
Copy link
Collaborator

@Nitvex Nitvex Sep 5, 2023

Choose a reason for hiding this comment

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

I propose to use a function takeEditorScreenshot. It automatically creates screenshot of canvas area and generates the screenshot name. (may be it requires some modification for Macromolecules)

Description: Rectangle Selection Tool
*/

await turnOnMacromoleculesEditor(page);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This function is, most likely, supposed to be used in beforeEach block as the tests are always for MacroMolecules

await page.mouse.click(500, 200);

// Get 4 peptides locators
const peptides = await page.getByText('Tza').locator('..');
Copy link
Collaborator

Choose a reason for hiding this comment

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

i propose to create a separate function for making some structure and reuse it across tests. Then it will be simpler to replace it, once we have import from file feature

}

export async function selectRectangleSelectionTool(page: Page) {
const bondToolButton = page.locator(`button[title*="select-rectangle"]`);
Copy link
Collaborator

Choose a reason for hiding this comment

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

My proposal is to avoid using locators by title and use page.getByTestId

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

All tools buttons are done in that way. Let's keep it in one style for now.

import { expect, Page } from '@playwright/test';

export async function turnOnMacromoleculesEditor(page: Page) {
await expect(page.getByTestId('PolymerToggler')).toBeVisible();
Copy link
Collaborator

Choose a reason for hiding this comment

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

I propose to maintain common style and use kebab-case for ids

@@ -51,6 +49,20 @@ export class CoreEditor {
this.events.selectMonomer.add((monomer) => this.onSelectMonomer(monomer));
this.events.selectPreset.add((preset) => this.onSelectRNAPreset(preset));
this.events.selectTool.add((tool) => this.onSelectTool(tool));
const renderersEvents: ToolEventHandlerName[] = [
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's move it to separate file. Most likely, we will have a lot of different events.

- added an ability  to add peptides on canvas and move by drag n drop
#2360 - "Select" tool

- added select and erase tools
- implemented DrawingEntitiesManager and RenderersManager which controls entities parameters changes and render
- added rootDir to tsconfig of core package
@rrodionov91 rrodionov91 merged commit ada37e4 into master Sep 13, 2023
3 of 4 checks passed
@rrodionov91 rrodionov91 deleted the 2370-erase-tool branch September 13, 2023 11:42
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.

"Erase" tool for macromolecules editor "Select" tool for Macromolecules editor
3 participants