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

TimeGraphNavigator snap on click feature #181

Conversation

williamsyang-work
Copy link
Contributor

@williamsyang-work williamsyang-work commented Feb 25, 2022

TimeGraphNavigator and TimeGraphVerticalScrollbar Snap On Click Feature

CPT2202251800-1149x423

Adds background PIXI rectangles to the time-graph-navigator + vertical-scrollbar components. This rectangle listens for clicks then centers the handler on the mouse and resumes 'dragging' for navigating.

Fixes: #97

Signed-off-by: Will Yang william.yang@ericsson.com

@williamsyang-work williamsyang-work changed the title Scrollbar snap on click feature TimeGraphNavigator snap on click feature Feb 26, 2022
@williamsyang-work williamsyang-work force-pushed the scrollbar-snap-on-click-feature branch 2 times, most recently from 1a93e81 to dad5e6b Compare February 26, 2022 00:27
this.stateController.snapped = false
}
const moveEnd = () => {
this.mouseIsDown = false
Copy link
Contributor

Choose a reason for hiding this comment

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

Use 4 space indentation. Comment applies to the whole PR.

Missing ; ?

timeline-chart/src/layer/time-graph-navigator.ts Outdated Show resolved Hide resolved
this.mouseIsDown = false
}
this.addEvent('mouseover', event => {
if(this.stateController.snapped) moveStart(event)
Copy link
Contributor

Choose a reason for hiding this comment

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

Space after if, use {} block for if-clause (with line break, indentation, ;).

this.addEvent('mouseupoutside', moveEnd, this._displayObject);
this.addEvent('mouseup', moveEnd, this._displayObject)
this.addEvent('mouseupoutside', moveEnd, this._displayObject)
document.addEventListener('snap-x-end', moveEnd)
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing ;

const start = BIMath.clamp(start0, min, max)
this.unitController.viewRange = {
start,
end: start + this.unitController.viewRangeLength,
Copy link
Contributor

Choose a reason for hiding this comment

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

indentation, unnecessary ,

}

protected toggleSnappedState = (bool: boolean) => {
this.stateController.snapped = bool;
Copy link
Contributor

Choose a reason for hiding this comment

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

Indentation

}

render(): void {
this.rect({
Copy link
Contributor

Choose a reason for hiding this comment

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

indentation

x: 0,
y: 0
},
width: Number(this.unitController.viewRangeLength),
Copy link
Contributor

Choose a reason for hiding this comment

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

The viewRangeLength is time in nanoseconds. I assume the width should be equal to the chart width in pixels?

Copy link
Contributor Author

@williamsyang-work williamsyang-work Mar 3, 2022

Choose a reason for hiding this comment

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

Yes. This has been changed to stateController.canvasDisplayWidth. Had to change some of the logic because of this (line 129).

y: 0
},
width: Number(this.unitController.viewRangeLength),
color: 0xe100ff,
Copy link
Contributor

Choose a reason for hiding this comment

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

Just curious, color value is irrelevant since opacity is 0, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, can't see color w/ opacity = 0. This was to visually see the background width. Color has been removed.

@williamsyang-work williamsyang-work force-pushed the scrollbar-snap-on-click-feature branch 4 times, most recently from c39aae9 to d018972 Compare March 3, 2022 18:01
Snap to mouse on time-graph-navigator click

Adds background PIXI rectangles to the time-graph-navigator +
vertical-navigator components.  This rectangle listens for clicks then centers the handler on the mouse and resumes 'dragging' for navigating.

Fixes: eclipse-cdt-cloud#97

Signed-off-by: Will Yang <william.yang@ericsson.com>
@williamsyang-work
Copy link
Contributor Author

@PatrickTasse I fixed the indentation, semicolons, and unnecessary commas in both files. I addressed the unique issues in replies to their comments!

Copy link
Collaborator

@bhufmann bhufmann left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thanks for the improvement.

@bhufmann bhufmann merged commit be37b18 into eclipse-cdt-cloud:master Mar 7, 2022
@williamsyang-work williamsyang-work deleted the scrollbar-snap-on-click-feature branch August 21, 2024 21:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support navigation in time by clicking on the horizontal scrollbar
3 participants