-
Notifications
You must be signed in to change notification settings - Fork 53
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
Introduce a Primer element #242
Conversation
@isaacguerreir this is epic. You killed it, this is dope. I made a couple small changes to address your points:
I don't like this part of SeqBlock, because it's so.. brittle and hard-coded, but it was a slightly misordered height calc in this block: seqviz/src/Linear/SeqBlock.tsx Lines 256 to 262 in 13fcc68
This was just ordering in the SeqBlock. I changed it so Highlight.Block is first and the Forward Primer is above it: <Selection.Block
findXAndWidth={this.findXAndWidth}
firstBase={firstBase}
fullSeq={fullSeq}
lastBase={lastBase}
selectHeight={selectHeight}
onUnmount={onUnmount}
/>
{primerFwdRows.length && (
<PrimeRows
bpsPerBlock={bpsPerBlock}
direction={1}
elementHeight={elementHeight}
findXAndWidth={this.findXAndWidthElement}
firstBase={firstBase}
fullSeq={fullSeq}
inputRef={inputRef}
lastBase={lastBase}
primerRows={primerFwdRows}
seqBlockRef={this}
width={size.width}
yDiff={primerFwdYDiff}
/>
)} |
@@ -141,6 +155,12 @@ export default class Linear extends React.Component<LinearProps> { | |||
if (zoomed) { | |||
blockHeight += showComplement ? lineHeight : 0; // double for complement + 2px margin | |||
} | |||
if (primerFwdRows[i].length) { | |||
blockHeight += primerFwdRows[i].length * lineHeight; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
because it's a row of rows, we have to multiple the row count by lineHeight (in case there's >1 rows of primers)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I didn't get it. Should I change something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No this and the other comments are just me sharing the things I changed as an FYI about some internal details
src/Linear/Primers.tsx
Outdated
}) => { | ||
return ( | ||
<g | ||
className="la-vz-linear-annotation-row" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here and in a couple other places we'll want "primers" in place of annotations
@@ -32,7 +32,16 @@ export interface Translation extends NameRange { | |||
direction: -1 | 1; | |||
} | |||
|
|||
/** Primer is a single primer for PCR. Not visualized right now. */ | |||
/** PrimerProp is a single primer to visualize above/below the linear viewer. */ | |||
export interface PrimerProp { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've been trying to make these *Prop version of arguments. They're like the regular thing, but as a "Prop" when passed as props they can be missing id/color, whereas Primer cannot
@@ -141,6 +155,12 @@ export default class Linear extends React.Component<LinearProps> { | |||
if (zoomed) { | |||
blockHeight += showComplement ? lineHeight : 0; // double for complement + 2px margin | |||
} | |||
if (primerFwdRows[i].length) { | |||
blockHeight += primerFwdRows[i].length * lineHeight; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No this and the other comments are just me sharing the things I changed as an FYI about some internal details
Should I make a Circle visual element? Or this is not imperative for accepting this PR? |
I think that'd be great, and probably part of the acceptance criteria for closing #232 But that we can merge this and release it without having it Circular (yet) One more small thing: could you please add docs on the |
Primer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
small fixes to sed replace annotations > primers
- change `annotation` to `primer` on component ids - fix typos Co-authored-by: Joshua Timmons <joshua.timmons1@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, this is cool!
Thanks for helping me, @jjti. I was talking today with @guzmanvig about how the code is definitely much simpler than it should be for the complexity it's taking it. So thanks for making so many comments and the cleanness of the Seqviz code. |
I made the component to visualize primers (#232).
So far, so good. Some problems I would like to ask for some help.