-
Notifications
You must be signed in to change notification settings - Fork 23
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
Fix statistics overview graph error #5113
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome 👍 /edit: see my other comment 🙈
@@ -90,7 +90,7 @@ class StatisticView extends React.PureComponent<{}, State> { | |||
|
|||
selectDataPoint = ({ chartWrapper }: GoogleCharts) => { | |||
const chart = chartWrapper.getChart(); | |||
const indicies = chart.getSelection().selection[0]; | |||
const indicies = chart.getSelection()[0]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you test this in multiple browsers? I'm wondering whether the library exposes an inconsistent behavior somewhere. Since 2019-02-14 we use "react-google-charts": "^2.0.0"
and on 2020-06-23, @MichaelBuessemeyer changed this exact line (see fe93f66#diff-823e1a6f8f8ef290cfeec2ab2219cc92dea4da813f1c1717b833b70346953b75R93). Now your change would basically revert his change 😄 I assume you both had good reasons to change the code, but they are contradicting each other. I'm afraid that we might get another bug report soon. So, it would be good to find out what's going on. Testing multiple browsers might be a good place to start 🤔
Maybe it also depends on where one clicks exactly or something like that?
@MichaelBuessemeyer Do you know more about this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just talked with Gritta about this change / bug and it looks like her versions is correct. Maybe I messed this up last year 😅 🦢?
We checked the fix in firefox and chromium. It would be nice if oucld also check the fix in the browsers you have installed @philippotto 🤖
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for doublechecking this! I also tested again with firefox and chrome and it seems to work. Let's hope this is settled then :)
This PR fixes the occurrence of an error when clicking on the graph of the statistics overview page.
URL of deployed dev instance (used for testing):
Steps to test:
Issues:
Updated (unreleased) migration guide if applicableUpdated documentation if applicableAdapted wk-connect if datastore API changesNeeds datastore update after deployment