Feat/translation handle

[guzmanvig]

solves #233

The way I solved this was by making the translation element take double the height, so half of it is used for the AAs and the other half for the handle.

@leshane the only requirement I didn't do was a flag to show or not show the handle. This is quite complex because it means that every other element (annotations, primers, etc) can have a different Y diff with respect to the translations. @jjti maybe you have a suggestion on how to do this?

[guzmanvig]

In the last commit I added a margin at the bottom to the translation handles. This was because if there were 2 on the same row, there was no space between them. The way I did the margin was by basically making the whole handle smaller. I also reduced the font.

Anyway, it's ready to review now.

[jjti]

@guzmanvig this looks really good, love the look of it. Couple nits/questions:

• can we give some space between the translation and the handle? Them touching seems slightly off/strange imo. cc @leshane for input/thoughts
• imo we should exclude the handle if no name is provided. Ie match the current behavior. For a couple reasons: 1. I just think it looks better w/o the handle vs adding a label given the range and 2. it matches the existing behavior so is a smaller change. So the ask here is: can we only show this handle if the name is set?

[guzmanvig]

@guzmanvig this looks really good, love the look of it. Couple nits/questions:

• can we give some space between the translation and the handle? Them touching seems slightly off/strange imo. cc @leshane for input/thoughts
• imo we should exclude the handle if no name is provided. Ie match the current behavior. For a couple reasons: 1. I just think it looks better w/o the handle vs adding a label given the range and 2. it matches the existing behavior so is a smaller change. So the ask here is: can we only show this handle if the name is set?

Thanks @jjti !

So both your questions were Kevin's decisions. I originally added a margin, but Kevin asked me to remove it. He also wanted the indices as the default name. I'll double check with him anyway.

However, regarding the second question, it is not trivial to implement. Each SeqBlock height and the position of every row in it is calculated based on the height of the rows in it. The way I calculate the translation row height right now is:

translationHeight = elementHeight * 2 * translationRows.length

(* 2 to account for both translation and handle)

If some of the translation rows have the handle but other's don't, it means the height of the translation row sometimes is "X" and sometimes is "X/2". So we cannot have a single translationHeight. Which affects other stuff like the next row Y:

// height and yDiff of annotations
const annYDiff = translationYDiff + translationHeight; 

or the selection height:

    // calc the height necessary for the sequence selection
    // it starts 5 above the top of the SeqBlock
    const selectHeight =
      primerFwdHeight +
      cutSiteHeight +
      indexHeight +
      compHeight +
      translationHeight +
      annHeight +
      primerRevHeight +
      elementGap +
      5;
    let selectEdgeHeight = selectHeight + 9; // +9 is the height of a tick + index row

In any case, I can try to change all the current logic to achieve this. Just giving the heads up on what this change implies.

[jjti]

Each SeqBlock height and the position of every row in it is calculated based on the height of the rows in it.

@guzmanvig my proposal wasn't to change the translation height but to add some margin to the handle element itself. So same height, just starting eg 10% into the height of the SVG element rather than 0%

[guzmanvig]

Each SeqBlock height and the position of every row in it is calculated based on the height of the rows in it.

@guzmanvig my proposal wasn't to change the translation height but to add some margin to the handle element itself. So same height, just starting eg 10% into the height of the SVG element rather than 0%

@jjti My explanation was regarding showing or not showing the handle based on the name. Not showing the handle would make the translation row have half of the height

[jjti]

@jjti My explanation was regarding showing or not showing the handle based on the name. Not showing the handle would make the translation row have half of the height

Ah yeah then makes sense and agreed w/ point of having to look thru each element in a row to check whether it's one or more rows

[jjti]

In any case, I can try to change all the current logic to achieve this.

Still not sure that "all the current logic" needs to change? That height calc is already per SeqBlock. It's just checking each row of translations to check if it needs to be 1 or 2 rows high right? Maybe that's what you meant, just asking to head off a large refactor that's probably not necessary

[guzmanvig]

In any case, I can try to change all the current logic to achieve this.

Still not sure that "all the current logic" needs to change? That height calc is already per SeqBlock. It's just checking each row of translations to check if it needs to be 1 or 2 rows high right? Maybe that's what you meant, just asking to head off a large refactor that's probably not necessary

Oh no, you are right, it might be easier that I initially thought. I'll check in with Kevin and if he's ok with not showing the indices I'll make the change

[guzmanvig]

@jjti talked to @leshane, we agreed that if no name is present we don't show the handle, and we added a small margin and a border to the handle. Let me know what you think

[jjti]

LGTM

[guzmanvig]

Thanks for the review @jjti. Will make a new release