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

Render gene with CDS subfeatures properly #2863

Merged
merged 1 commit into from
Apr 25, 2022
Merged

Render gene with CDS subfeatures properly #2863

merged 1 commit into from
Apr 25, 2022

Conversation

cmdcolin
Copy link
Collaborator

Possible fix for #1887

before
Screenshot from 2022-03-29 17-28-12

after
Screenshot from 2022-03-29 17-27-54

this feature has CDS subfeature so triggers "hasSubSub" in the util

There are a number of weird workarounds here, but it does solve the proximate problem

Sort of unrelated but I think it may be worth making the layout part of a pre-processing routine. The way the code makes the layout as a side product of rendering might not be ideal

@github-actions github-actions bot added the needs label triage Needs a label to show in changelog (breaking, enhancement, bug, documentation, or internal) label Mar 29, 2022
@@ -25,7 +25,8 @@ function Box(props: {
const { feature, region, config, featureLayout, bpPerPx, topLevel } = props
const { start, end } = region
const screenWidth = (end - start) / bpPerPx
const { left = 0, width = 0 } = featureLayout.absolute
const width = (feature.get('end') - feature.get('start')) / bpPerPx
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this is an interesting issue because I was not able to trust the feature layout's calculation of width. it might be that it needs further investigation

@cmdcolin cmdcolin marked this pull request as draft March 29, 2022 23:34
@cmdcolin cmdcolin added bug Something isn't working and removed needs label triage Needs a label to show in changelog (breaking, enhancement, bug, documentation, or internal) labels Mar 29, 2022
@codecov
Copy link

codecov bot commented Mar 29, 2022

Codecov Report

Merging #2863 (96dfb55) into main (d485541) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #2863   +/-   ##
=======================================
  Coverage   59.92%   59.92%           
=======================================
  Files         590      590           
  Lines       26573    26574    +1     
  Branches     6450     6450           
=======================================
+ Hits        15923    15925    +2     
+ Misses      10346    10345    -1     
  Partials      304      304           
Impacted Files Coverage Δ
...gins/svg/src/SvgFeatureRenderer/components/Box.tsx 100.00% <100.00%> (ø)
...gins/svg/src/SvgFeatureRenderer/components/util.ts 91.83% <100.00%> (ø)
...ckages/core/pluggableElementTypes/RpcMethodType.ts 85.41% <0.00%> (+2.08%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d485541...96dfb55. Read the comment docs.

@cmdcolin
Copy link
Collaborator Author

cmdcolin commented Apr 5, 2022

while the code is very workaround-y, it fixes a real issue and alternatives may require a fairly large refactoring. so, we could accept this as is perhaps

larger refactoring might involve making a "Gene" glyph and only doing subsubfeatures for that, but that may require modifications to how chooseGlyph works and then only doing changes in vertical alignment on a Gene feature (currently on main it is sort of trying to add "gene-type glyphs in the middle of a transcript)

Copy link
Collaborator

@garrettjstevens garrettjstevens left a comment

Choose a reason for hiding this comment

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

I think this is good, I agree we can do larger refactoring later if needed.

@cmdcolin cmdcolin marked this pull request as ready for review April 25, 2022 20:52
@cmdcolin cmdcolin merged commit fb58a99 into main Apr 25, 2022
@cmdcolin cmdcolin deleted the subsubfeature branch April 26, 2022 13:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants