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.

Sign up for GitHub

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

Further Time Tacking Improvement #4121

Merged
merged 19 commits into from
Jul 2, 2019
Merged

Further Time Tacking Improvement #4121

merged 19 commits into from
Jul 2, 2019

Conversation

MichaelBuessemeyer
Copy link
Contributor

@MichaelBuessemeyer MichaelBuessemeyer commented Jun 4, 2019 •
edited
Loading

This PR adds the following suggest improvements to the time tracking view:

  • The format of the duration in the tooltip is now minutes:seconds
  • A spinning wheel is showing up when the data is being fetched from the backend and rendered into a diagram
  • There seems to be a bug that causes tooltips to not being rendered. Look at the discuss for further clarification

URL of deployed dev instance (used for testing):

Steps to test:

  • Get some tracking data by e.g. tracing.
  • open the time tracking view and select the correct user and date.
  • When the data is fetched by the server, there should be a spinning wheel visible. But it is not visible, while the google charts library is actually rendering the chart.
  • The tooltip should always be next to the mouse (if visible). It should not appear somewhere else and then "beam" next to the mouse.
  • The duration displayed in the tooltip should have the format of minutes:seconds.
  • Also please consider that the tooltip might be partly positioned out of the window. I added a fix that adjusts the position for these cases. Please also try to whether you can find a case where the tooltip is still partly placed outside the window.

Issues:

@MichaelBuessemeyer MichaelBuessemeyer self-assigned this Jun 4, 2019
@MichaelBuessemeyer
Copy link
Contributor Author

@philippotto This PR is ready for your first review 🙂

@MichaelBuessemeyer MichaelBuessemeyer changed the title [WIP] Further Time Tacking Improvement Further Time Tacking Improvement Jun 17, 2019
@philippotto
Copy link
Member

I just tested the branch on https://improvetimetracking.webknossos.xyz/reports/timetracking but for me no tooltip is rendered? Does it work for you on the dev instance?

@MichaelBuessemeyer
Copy link
Contributor Author

MichaelBuessemeyer commented Jun 20, 2019 •
edited
Loading

I just tested the branch on https://improvetimetracking.webknossos.xyz/reports/timetracking but for me, no tooltip is rendered? Does it work for you on the dev instance?

Yes, it does not work for me too. The bug seems to appear only if you have few data rows 🤔

@MichaelBuessemeyer
Copy link
Contributor Author

I just tested the branch on https://improvetimetracking.webknossos.xyz/reports/timetracking but for me, no tooltip is rendered? Does it work for you on the dev instance?

I found a workaround for this -> see my comments in the code

MichaelBuessemeyer
MichaelBuessemeyer commented Jun 20, 2019
View reviewed changes
frontend/javascripts/admin/time/time_line_chart_view.js
// We need to make the tooltip visible again after adjusting the position.
// It is initially invisible as it is mispositioned by the library and would then "beam" to the corrected
// position. We want to avoid that "beaming" behaviour.
tooltip.style.visibility = "visible";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The "beaming" behavior e.g. happens in Firefox for me. Chrome works fine even without the workaround.
If you are looking where the tooltip is set to be invisible at the beginning: it is done in the google-charts-overwrites.less

MichaelBuessemeyer
MichaelBuessemeyer commented Jun 20, 2019
View reviewed changes
frontend/javascripts/admin/time/time_line_chart_view.js
graph_id="TimeLineGraph"
chartPackages={["timeline"]}
width="100%"
height="600px"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@philippotto: We need a fixed height as the workaround we found, that displays the chart at full height, leaves out the whole y-axis with the actual dates. Having a fixed height solves this.
I also shortly tried to force the y-axis to be rendered below the chart but failed :/

@MichaelBuessemeyer
Copy link
Contributor Author

@philippotto This PR should be ready for your next review. I hopefully fixed the bug that causes the tooltip, not to render,

@MichaelBuessemeyer
Copy link
Contributor Author

@philippotto I think I still need your review on this PR.

philippotto
philippotto approved these changes Jun 27, 2019
View reviewed changes
Copy link
Member

@philippotto philippotto left a comment

Choose a reason for hiding this comment

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

Awesome 👍 The workaround works for me in Firefox and Chrome 👍

@MichaelBuessemeyer MichaelBuessemeyer merged commit 2eef350 into master Jul 2, 2019
@normanrz normanrz deleted the improve-time-tracking branch August 12, 2019 09:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@philippotto philippotto philippotto approved these changes

Assignees

@MichaelBuessemeyer MichaelBuessemeyer

Labels
bug enhancement frontend
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Improve time tracking
2 participants
@MichaelBuessemeyer @philippotto