#2332 - Add peptide monomers to canvas (by click) #2995
Conversation
rrodionov91
requested review from
AleksandraSmolianinova,
Ustianskii,
Vehemen,
Zhirnoff,
y-holik,
MartaWilliams and
krakmielepam
as code owners
rrodionov91
force-pushed
the
2332-add-peptides-to-canvas
branch
4 times, most recently
from
fd0ddde to
c69dce2
Compare
| return true; | ||
| } | ||
|
|
||
| // if (eventName !== 'mouseup' && eventName !== 'mouseleave') { |
There was a problem hiding this comment.
do we need this comment?
| return true; | ||
| } | ||
|
|
||
| return true; |
There was a problem hiding this comment.
So basically, this function return true in any case. Is that ok?
Also, why does it return value at all?
There was a problem hiding this comment.
I copy pasted it from ketcher-react Editor. In future we are going to replace it by ketcher-core Editor.
| if (!editorTool) { | ||
| return false; | ||
| } | ||
| // this.lastEvent = event; |
There was a problem hiding this comment.
do we need this comments?
There was a problem hiding this comment.
Same here. It will be uncommented in future
| restruct.peptides.set(newPeptide.id, peptideRenderer); | ||
| } | ||
|
|
||
| invert() { |
There was a problem hiding this comment.
what for do we use this method?
Actually, maybe you can add some short comments about main methods and their purpose, where applicable? I don't insist, just idea. In this PR are a lot new methods that will be used somewhere in the future, so maybe some short explanation will help. Is it ok?
There was a problem hiding this comment.
Operations are a part of regular ketcher. I just used the same interface
There was a problem hiding this comment.
For sure it should be documented, however I'm not sure that in this PR, because it is quite big and the main part of it is related to peptides mainly
| class PeptideTool implements Tool { | ||
| private peptidePreview: Peptide | undefined; | ||
| private peptidePreviewRenderer: PeptideRenderer | undefined; | ||
| readonly PEPTIDE_PREVIEW_SCALE_FACTOR = 0.4; |
There was a problem hiding this comment.
Why this exact numbers? Just curious)
There was a problem hiding this comment.
To match the design. Preview is less than a regular peptide in this scale factor
| @@ -652,6 +664,13 @@ class ReStruct { | |||
| }); | |||
| } | |||
|
|
|||
| private showPeptides(): void { | |||
| this.peptides.forEach((peptideRenderer) => { | |||
| peptideRenderer.remove(); | |||
There was a problem hiding this comment.
why do we first remove it?
There was a problem hiding this comment.
We need it to rerender. In future we will be able to rerender only parts which are actually need to be rerendered, not all ones
| > | ||
| <defs> | ||
| <symbol id="peptide" viewBox="0 0 70 61" width="70" height="61"> | ||
| <path d="M16.9236 1.00466C17.2801 0.383231 17.9418 6.10888e-07 18.6583 5.98224e-07L51.3417 2.04752e-08C52.0582 7.81036e-09 52.7199 0.383234 53.0764 1.00466L69.4289 29.5047C69.7826 30.1211 69.7826 30.8789 69.4289 31.4953L53.0764 59.9953C52.7199 60.6168 52.0582 61 51.3417 61H18.6583C17.9418 61 17.2801 60.6168 16.9236 59.9953L0.571095 31.4953C0.217407 30.8789 0.217408 30.1211 0.571096 29.5047L16.9236 1.00466Z"></path> |
There was a problem hiding this comment.
Idk, maybe move it to separate file? I looks not very beautiful here xD
There was a problem hiding this comment.
Yeah good idea. Let me do it in the nest pr related to this topic
rrodionov91
force-pushed
the
2332-add-peptides-to-canvas
branch
from
c69dce2 to
3a5cd1f
Compare
- added an ability to add peptides on canvas and move by drag n drop
rrodionov91
force-pushed
the
2332-add-peptides-to-canvas
branch
from
3a5cd1f to
1c6f094
Compare
|
|
||
| class SelectLasso implements Tool { | ||
| private isMouseDown = false; | ||
| private selectedItem; |
| this._position.y += position.y; | ||
| } | ||
|
|
||
| public moveAbsolute(position: Vec2) { |
There was a problem hiding this comment.
consider change to point
|
|
||
| public get textColor() { | ||
| const WHITE = 'white'; | ||
| const colorsMap = { |
| return canvas | ||
| .append('g') | ||
| .data([this]) | ||
| .attr('transition', 'transform 0.2s') |
| this.canvas = select<SVGElement, unknown>('#polymer-editor-canvas'); | ||
| } | ||
|
|
||
| public abstract show(theme): void; |
Closes #2332
How the feature works? / How did you fix the issue?
Screen.Recording.2023-07-28.at.16.36.05.mov
Check list
#1234 – issue name