Center rows when selected #211
Conversation
There was a problem hiding this comment.
Looks good to me. @PatrickTasse could you please have review in respect of the visible rows and background fetching implementation.
| protected navigate(rowIndex: number) { | ||
| this.ensureVisible(rowIndex); | ||
| protected navigate(rowIndex: number, center?: boolean) { | ||
| center ? this.centerRow(rowIndex) : this.ensureVisible(rowIndex); |
There was a problem hiding this comment.
I think that the center flag should be propagated to ensureVisible() where it would be used only when a row needs to be revealed. That is, if calling selectAndReveal() on a row that is already visible, I don't think the row should be centered and the vertical offset should be unchanged. The user might have scrolled to put the focused row in a specific desired place.
Also, perhaps the trigger of this issue is that the horizontal scroll bar of the left tree can hide the text of the last displayed tree row. The bottom left corner of the timegraph output component is unused, I wonder if it could be used for the tree's horizontal scroll bar? (This is a separate issue, on theia-trace-extension).
There was a problem hiding this comment.
I didn't realize navigate was called even when a row is clicked in the timeline-chart. That is annoying behavior... Nice catch.
There was a problem hiding this comment.
For the horizontal scrollbar issue, that's only applicable if only one chart component is open. IE, if we have multiple open, that only applies to the bottom one.
Signed-off-by: Will Yang <william.yang@ericsson.com>
williamsyang-work
force-pushed
the
center-rows-when-selected
branch
from
40e92f8 to
d979c43
Compare
Adds the method TimeGraphChart#centerRow which has logic to adjust the vertical offset to center a row given it's index.
Fixes: theia-trace-extension #773
Signed-off-by: Will Yang william.yang@ericsson.com