-
Notifications
You must be signed in to change notification settings - Fork 0
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
Refactor axis to use common implementation for x and y #140
Conversation
Refactor to group by type of axis (linear/time/ordinal) rather than by direction. Use a common implementation for each chart for creating an axis, based on the settings and data it will be working from. Allow for aggregates that are strings turned into numbers (e.g. `count`) or left as string (e.g. `dominant`)
db4c5f8
to
43e93f5
Compare
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.
Looking good so far.
I have a couple of observations:
-
The vertical gridlines have been removed for heatmaps. Is this intentional to fix issue (https://int.scottlogic.co.uk/jira/browse/IPJS-88)?
-
Integer numbers are now formatted so that they have a comma for thousands. e.g. 1,100.
This has an unfortunate effect when the cross axis is a year. E.g. 2000, 2002, 2004 become 2,000; 2,002 etc:
-
The height of the heatmap seems to have been affected in some instances:
http://localhost:3000/streaming/streaming.heatmap.html
- For the specific example that Andrew referenced in his issue:
[D3FC] Non-numeric axes on X/Y scatter charts finos/perspective#499
The chart now displays points, which is great. The sub-category axis, however, is not quite correct as the lines do not line up with the labels correctly:
I have also ran the puppeteer tests. Most passed, some of the heatmaps failed because of the vertical lines being removed, but some also some tests failed because the numbers on the legend seem to have changed slightly. I assume the underlying data hasn't changed, so it is slightly odd that this has happened. I am not sure if it indicates a problem: |
(5). I cannot see any reason for the change in heatmap colour range. However, I am planning to change that soon anyway (in a separate PR), since it will look better with a |
Also, I'll have to take your word for the puppeteer tests, since I can't run any of them anymore. |
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 really like the axisFactory and the chartFactory it makes it very neat.
Instead of organising by "cross" and "main" axis, we now arrange the code by type of axis (time/linear/ordinal).
The type is picked by a common "factory" implementation, leading to a set of X-axis affecting components which can then be passed into the chart using a reusable chart "factory".