Implement Wallet Graph component #16789
Conversation
Jenkins BuildsClick to see older builds (70)
|
briansztamfater
force-pushed
the
research/chart-library
branch
9 times, most recently
from
d31d813
to
19536c8
Compare
briansztamfater
changed the title
briansztamfater
force-pushed
the
research/chart-library
branch
2 times, most recently
from
201b127
to
b102c1b
Compare
briansztamfater
assigned rasom, smohamedjavid and OmarBasem and unassigned rasom, smohamedjavid and OmarBasem
| (defn downsample-data | ||
| [data max-array-size] | ||
| (let [data-size (count data) | ||
| aggregation-interval (max (/ data-size max-array-size) 1)] |
There was a problem hiding this comment.
It looks like we are cutting out the most recent portion of data if it doesn't fit into max-array-size, e.g. if we have data=[1 2 3 4 5] and max-array-size=2 then only [1 2 3] are going to be used. Not a big deal on a big set, but we still would rather want to cut the head of that set in case if the set is ordered from the older value to newer (which is likely).
There was a problem hiding this comment.
I've refactored and simplified the function and added some unit test so expected outputs are more understandable. Basically it cuts the array into by X amount of steps to fit given max amount of elements (if the original data array exceeds that amount) and avoid rendering lots of unnecessary points which won't even be visible in mobile screens.
If I did not misunderstood your suggestion, I think in that case we could split the data array and pre-process it, and keep this function as simple as possible. Let me know if that makes sense.
There was a problem hiding this comment.
Do we know what are expected numbers for (count data) and max-array-size? I do like that a new implementation is shorter but it might have introduced another issue depending on data we are going to show.
For instance, in the previous implementation we would cut the last portion of data if it wouldn't have enough elements in it, e.g. for data=[1 2 3 4 5] and max-array-size=2 if would be split into [1 2 3] and [3 4] and then [3 4] would not be shown, which in some cases might mean we do not show the recent data, and that might not be optimal (specifically if the user wants to see that recent data and the dataset is relatively small).
In a new implementation by doing decimation we can get a noticeably lower number of elements in the resulting data set although we potentially have enough information to show more details of it. e.g. if we have (count data) = 21 and max-array-size=20 the result will have only 11 elements. That's not going to be a problem if that dataset is big enough (comparing to max-array-size) or if (count data) is multiple of max-array-size.
That's why I'm asking whether we know what to expect here :)
There was a problem hiding this comment.
For clarification, this function is only meant to use with big data sets (> 500 points). As far as I've tested, 500 points at max works fine and is performant. (count data) could be any value, maybe price history of a token, but for the preview I am using random datasets of 182500 elements (which would not be performant to render all and there's no space in the mobile screen to render them either way, so it is downsampled to 500 elements). In reality, I think user could view other smaller timeframes to inspect detailed information.
Anyways, I will create an issue so we can revisit this later once we start testing it with real data sets.
There was a problem hiding this comment.
Created it here #16831
briansztamfater
force-pushed
the
research/chart-library
branch
from
e8c1d5a
to
10a3c7a
Compare
There was a problem hiding this comment.
Who doesn't love line charts ;) Cool work @briansztamfater, and the unit tests made it all better!
I actually had fun times developing this component! And I must admit I now feel better after adding those unit tests, so thanks for the valuable feedback 💪 I think we can now safely summon @Francesca-G to review this component 🧙 |
Signed-off-by: Brian Sztamfater <brian@status.im>
briansztamfater
force-pushed
the
research/chart-library
branch
from
05bebaf
to
26597d1
Compare
fixes #16739 #16705
Summary
Integrate
react-native-gifted-chartslibrary and implement Wallet Graph componentResults
wallet-graph-light.mp4
walletgraph-dark.mp4
Platforms
Areas that maybe impacted
Steps to test
wallet-graphcomponent underGraphsectionstatus: ready