Conversation
|
|
joko3ono
force-pushed
the
joko/fix-results-flickering
branch
4 times, most recently
from
c0f512a to
af00a3c
Compare
tadast
left a comment
tadast
left a comment
There was a problem hiding this comment.
First of all, I really like that hits got their own component now, makes the code a bit easier to follow, thanks 🙌 Overall it looks good, but to get this over the finish line we need to fix the legitimate spec failures in the suite:
Also a couple suggestions in the comments.
| if (this.nextQuery == 0 && this.nextHit == 0 && this.nextHSP == 0) { | ||
| this.affixSidebar(); | ||
| } |
There was a problem hiding this comment.
Should this be a responsibility of report.js? I think now with the custom hits batch-rendering logic separated, we should be able to affix the sidebar one in report.js without any of the batch-rendering related checks or am I missing something?
There was a problem hiding this comment.
@tadast from my perspective it's better this way
in report js, we already have 2 checks
- if a warning present
- if results available
| <div className='histogram-container'> | ||
| <Histogram query="evalue" name="E-Value Distribution" color={colors[0]} /> | ||
| </div> |
There was a problem hiding this comment.
Would it be possible to clean up this PR to only inlcude the performance fix without the Histogram changes?
There was a problem hiding this comment.
can we keep it?
this PR is required to fix performance before we merge histogram.
joko3ono
force-pushed
the
joko/fix-results-flickering
branch
from
de40021 to
ef8c828
Compare
3 participants
Context
When I created the histogram, I found that the diagram (circos and histogram) flickered for up to 4 minutes or more, when it flickers, I can't inspect the diagram because it keeps rendering.
Screen.Recording.2024-06-12.at.13.35.31.mov
Solution
In this PR, I separated the diagram and results rendering so that the diagram will only render once.
Screen.Recording.2024-06-12.at.13.39.46.mov