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

feat: add option for stacked line layers #119

Merged
merged 1 commit into from
Nov 26, 2019

Conversation

TCL735
Copy link
Contributor

@TCL735 TCL735 commented Nov 25, 2019

Closes #43

@TCL735 TCL735 force-pushed the feature_43_stacked_line_layers branch 3 times, most recently from 37bfba7 to e031438 Compare November 25, 2019 18:35
@TCL735
Copy link
Contributor Author

TCL735 commented Nov 25, 2019

Example of stacked line layer. Notice also the tooltip has an additional column named "cumulative" which reflects the value of the hovered points, while the "_value" is the original single values of the points.

Screen Shot 2019-11-25 at 10 37 04 AM

@TCL735
Copy link
Contributor Author

TCL735 commented Nov 25, 2019

With shading

Screen Shot 2019-11-25 at 10 42 51 AM

Copy link
Contributor

@hoorayimhelping hoorayimhelping left a comment

Choose a reason for hiding this comment

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

this is really awesome timmy!

i particularly love this:

Notice also the tooltip has an additional column named "cumulative" which reflects the value of the hovered points, while the "_value" is the original single values of the points.

just some requests to shore up some potential undefined accesses. i think this is deserving of some tests as well - i commented where i think some might be good to put.

giraffe/src/transforms/line.ts Show resolved Hide resolved
giraffe/src/utils/lineData.ts Show resolved Hide resolved
giraffe/src/utils/tooltip.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@ebb-tide ebb-tide left a comment

Choose a reason for hiding this comment

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

I love it! 🍰 🍰 🍰

giraffe/src/transforms/line.ts Outdated Show resolved Hide resolved
giraffe/src/types/index.ts Outdated Show resolved Hide resolved
@TCL735
Copy link
Contributor Author

TCL735 commented Nov 26, 2019

Aside from the reviews, I noticed upon further testing that sometimes a line's data will not be in the tooltip when the _value is 0. This happens intermittently, as sometimes the tooltip will correctly display the entire line's data when _value is 0. Will investigate this and report back.

@TCL735
Copy link
Contributor Author

TCL735 commented Nov 26, 2019

Aside from the reviews, I noticed upon further testing that sometimes a line's data will not be in the tooltip when the _value is 0. This happens intermittently, as sometimes the tooltip will correctly display the entire line's data when _value is 0. Will investigate this and report back.

This is a data issue. The sample test data does not have enough records to allow the hover points to be found with high precision when a line has a _value 0 (and thus causing two line layers to intersect when stacked). Increasing the number of records in the sample data makes this issue un-reproduceable. Intersecting lines will have their own hover points.

In real applications, this issue is moot as the data volume is expected to be much higher than the sample test data in storybook.

@TCL735 TCL735 force-pushed the feature_43_stacked_line_layers branch from 52ad452 to 7d48c5b Compare November 26, 2019 22:02
@TCL735 TCL735 force-pushed the feature_43_stacked_line_layers branch from 7d48c5b to a1e37cb Compare November 26, 2019 22:18
Copy link
Contributor

@hoorayimhelping hoorayimhelping left a comment

Choose a reason for hiding this comment

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

Nice work, as usual! Top quality code!

@@ -28,13 +51,17 @@ export const lineTransform = (
for (let i = 0; i < table.length; i++) {
Copy link
Contributor

Choose a reason for hiding this comment

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

not a blocker, just a thought for the future: might consider converting this to forEach function if at all possible and unless there's a strong reason to use a for loop - manually iterating is sooooooooo 1989

@TCL735 TCL735 merged commit 268220d into master Nov 26, 2019
@TCL735 TCL735 deleted the feature_43_stacked_line_layers branch February 21, 2020 22:30
@TCL735 TCL735 mentioned this pull request Oct 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support stacked position option in line layers
3 participants