-
-
Notifications
You must be signed in to change notification settings - Fork 21
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
add faceting variable to g_lineplot. add code to maintain factor levels. #1226
Conversation
Code Coverage Summary
Diff against main
Results for commit: 867d907 Minimum allowed coverage is ♻️ This comment has been updated with latest results |
Unit Test Performance DifferenceAdditional test case details
Results for commit df8e348 ♻️ This comment has been updated with latest results. |
CLA Assistant Lite bot ✅ All contributors have signed the CLA |
I have read the CLA Document and I hereby sign the CLA |
Unit Tests Summary 1 files 83 suites 1m 10s ⏱️ Results for commit 867d907. ♻️ This comment has been updated with latest results. |
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.
Hi @ayogasekaram,
Thanks for working on this! I generated the updated test snapshots so we can compare the results. It looks pretty good so far, just a few questions from me:
- I'm not sure if this is a problem, but I think something you changed affected the y limits (which caused a slight change to 2 of the snapshots). Was this intentional?
- Now that there is an option to facet, plots could potentially be much more squished together horizontally (which you can see in the new test snapshot). Do you think we should also add a parameter to rotate the x-axis labels for in these cases?
I have read the CLA Document and I hereby sign the CLA |
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.
Lgtm!
Approved. |
closes #1212