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

Jump to bottom

Implement navigation for XY charts #419

Merged
merged 3 commits into from Sep 16, 2021
Merged

Implement navigation for XY charts #419

merged 3 commits into from Sep 16, 2021

Conversation

IbrahimFradj
Copy link
Contributor

@IbrahimFradj IbrahimFradj commented Jul 15, 2021 •
edited

Navigation implemented :

  • Zoom-in by following the mouse cursor : "w","W" and "ctrl+mouse wheel up".
  • Zoom-out by following the mouse cursor : "s", "S", and ""ctrl+mouse wheel down".
  • Zooming with range : click right and drag.
  • Panning left : "a","A", "arrowLeft" and "shift+mouse wheel up".
  • Panning right : "d","D", "arrowRight" and "shift+mouse wheel down".

Fixes theia-ide#389

Here is the result
zoom

Signed-off-by: Ibrahim Fradj ibrahim.fradj@ericsson.com

MatthewKhouzam
MatthewKhouzam reviewed Jul 16, 2021
View reviewed changes
Copy link
Contributor

@MatthewKhouzam MatthewKhouzam left a comment

Choose a reason for hiding this comment

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

Commenting, these are requested changes but I don't want to slow you down.

packages/react-components/src/components/xy-output-component.tsx Outdated Show resolved Hide resolved
packages/react-components/src/components/xy-output-component.tsx Outdated Show resolved Hide resolved
packages/react-components/src/components/xy-output-component.tsx Outdated Show resolved Hide resolved
packages/react-components/src/components/xy-output-component.tsx Outdated Show resolved Hide resolved
packages/react-components/src/components/xy-output-component.tsx Outdated Show resolved Hide resolved
packages/react-components/src/components/xy-output-component.tsx Outdated Show resolved Hide resolved
packages/react-components/src/components/xy-output-component.tsx Outdated Show resolved Hide resolved
packages/react-components/src/components/xy-output-component.tsx Outdated Show resolved Hide resolved
@IbrahimFradj IbrahimFradj requested review from chertyb and ebugden and removed request for chertyb July 16, 2021 14:39
ebugden
ebugden reviewed Jul 20, 2021
View reviewed changes
Copy link
Contributor

@ebugden ebugden left a comment

Choose a reason for hiding this comment

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

I have a lot of feelings about the amount of unrelated whitespace changes mixed in with the commit 😅 I think this is worth correcting before merging for learning purposes (creating clean and specific commits) and also to set a good style example with the commits that are merged.

That being said, the code seems to do what it claims to do! Thanks @IbrahimFradj ☀️

packages/react-components/src/components/xy-output-component.tsx Outdated Show resolved Hide resolved
packages/react-components/src/components/xy-output-component.tsx Outdated Show resolved Hide resolved
packages/react-components/src/components/xy-output-component.tsx Outdated Show resolved Hide resolved
packages/react-components/src/components/xy-output-component.tsx Outdated Show resolved Hide resolved
packages/react-components/src/components/xy-output-component.tsx Outdated Show resolved Hide resolved
packages/react-components/src/components/xy-output-component.tsx Outdated Show resolved Hide resolved
packages/react-components/src/components/xy-output-component.tsx Outdated Show resolved Hide resolved
packages/react-components/src/components/xy-output-component.tsx Outdated Show resolved Hide resolved
packages/react-components/src/components/xy-output-component.tsx Outdated Show resolved Hide resolved
packages/react-components/src/components/xy-output-component.tsx Outdated Show resolved Hide resolved
@IbrahimFradj IbrahimFradj force-pushed the IF-Zoom-Shift branch 2 times, most recently from e7b2bef to a44369f Compare July 23, 2021 21:15
@IbrahimFradj IbrahimFradj force-pushed the IF-Zoom-Shift branch 2 times, most recently from 0a4b919 to ebc3869 Compare July 27, 2021 15:12
@IbrahimFradj IbrahimFradj force-pushed the IF-Zoom-Shift branch 2 times, most recently from 179500e to f1a93be Compare July 28, 2021 00:04
@ebugden ebugden added this to High priority in Navigate and read a chart Jul 29, 2021
@IbrahimFradj IbrahimFradj force-pushed the IF-Zoom-Shift branch 4 times, most recently from e8de9a8 to 9095858 Compare July 29, 2021 21:06
@IbrahimFradj IbrahimFradj force-pushed the IF-Zoom-Shift branch 2 times, most recently from 9e60799 to fdcc094 Compare August 2, 2021 23:26
@IbrahimFradj IbrahimFradj requested a review from a user August 4, 2021 19:56
PatrickTasse
PatrickTasse requested changes Aug 5, 2021
View reviewed changes
packages/react-components/src/components/xy-output-component.tsx Outdated Show resolved Hide resolved
packages/react-components/src/components/xy-output-component.tsx Outdated Show resolved Hide resolved
packages/react-components/src/components/xy-output-component.tsx Outdated Show resolved Hide resolved
packages/react-components/src/components/xy-output-component.tsx Outdated Show resolved Hide resolved
packages/react-components/src/components/xy-output-component.tsx Outdated Show resolved Hide resolved
packages/react-components/src/components/xy-output-component.tsx Outdated Show resolved Hide resolved
packages/react-components/src/components/xy-output-component.tsx Outdated Show resolved Hide resolved
packages/react-components/src/components/xy-output-component.tsx Show resolved Hide resolved
packages/react-components/src/components/xy-output-component.tsx Outdated Show resolved Hide resolved
packages/react-components/src/components/xy-output-component.tsx Outdated Show resolved Hide resolved
@IbrahimFradj IbrahimFradj force-pushed the IF-Zoom-Shift branch 3 times, most recently from a381aa6 to 55c4dcc Compare September 3, 2021 21:25
PatrickTasse
PatrickTasse requested changes Sep 8, 2021
View reviewed changes
packages/react-components/src/components/xy-output-component.tsx Outdated Show resolved Hide resolved
packages/react-components/src/components/xy-output-component.tsx Outdated Show resolved Hide resolved
packages/react-components/src/components/xy-output-component.tsx Outdated Show resolved Hide resolved
packages/react-components/src/components/xy-output-component.tsx Outdated Show resolved Hide resolved
packages/react-components/src/components/xy-output-component.tsx Outdated Show resolved Hide resolved
packages/react-components/src/components/xy-output-component.tsx Outdated Show resolved Hide resolved
packages/react-components/src/components/xy-output-component.tsx Outdated Show resolved Hide resolved
packages/react-components/src/components/xy-output-component.tsx Show resolved Hide resolved
@IbrahimFradj IbrahimFradj force-pushed the IF-Zoom-Shift branch 3 times, most recently from e540cdd to c2807bd Compare September 15, 2021 19:01
PatrickTasse
PatrickTasse previously approved these changes Sep 15, 2021
View reviewed changes
@IbrahimFradj
Implement navigation for XY charts
7e2ea5e 
We can zoom in/out with "w"/"s".

We can pan left with "a","shift+mouse wheel up","ArrowLeft"

We can pan right with "d","shift+ mouse wheel down","ArrowRight".

Contributes towards fixing theia-ide#389

Signed-off-by: Ibrahim Fradj <ibrahim.fradj@ericsson.com>
@IbrahimFradj
zoom in and out with ctrl+wheel
2ad0261 
 add listner wheel and add prevent default to zoom in and out just in the component

Contributes towards fixing theia-ide#389

Signed-off-by: Ibrahim Fradj <ibrahim.fradj@ericsson.com>
@IbrahimFradj
Zoom in and out by click right drag
6a15e94 
We can now click right and drag to zoom in xy-charts

 Contributes towards fixing theia-ide#389

Signed-off-by: Ibrahim Fradj <ibrahim.fradj@ericsson.com>
bhufmann
bhufmann approved these changes Sep 15, 2021
View reviewed changes
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.

LGTM. Great improvement. Thank you very much!

@PatrickTasse PatrickTasse merged commit bbd7afc into eclipse-cdt-cloud:master Sep 16, 2021
Navigate and read a chart automation moved this from High priority to Closed Sep 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ebugden ebugden ebugden left review comments

@bhufmann bhufmann bhufmann approved these changes

@PatrickTasse PatrickTasse PatrickTasse approved these changes

@MatthewKhouzam MatthewKhouzam Awaiting requested review from MatthewKhouzam

@Rodrigoplp-work Rodrigoplp-work Awaiting requested review from Rodrigoplp-work

Assignees
No one assigned
Labels
None yet
Projects
Navigate and read a chart
Closed
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

XY Charts: Allow navigation like Time Graph
7 participants
@IbrahimFradj @mirsky-work @PatrickTasse @bhufmann @MatthewKhouzam @ebugden @Rodrigoplp-work