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

Add option to show active points on line and multi-lines charts #827

Conversation

thomaschampagne
Copy link
Contributor

This PR includes:

  • Option to show active points on line and multi-lines charts
  • Documentation samples to use this new feature.

Screenshot of the added documentation (tab "data"):
image

Hope this helps!

Thomas

@wlach
Copy link
Collaborator

wlach commented Mar 5, 2018

Thanks for the patch! I will try to make some time to look at it tomorrow.

@hamilton hamilton requested review from wlach and hamilton March 12, 2018 18:07
Copy link
Collaborator

@hamilton hamilton 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 such a great PR @thomaschampagne! I had one small nit re: adding unneeded circles to the DOM I'm hoping can get fixed (something we'll have to correct elsewhere in the codebase) but it is otherwise a fantastic addition.

if (args.active_point_on_lines) {

svg.selectAll('circle-' + line_id)
.data(args.data[i])
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a way we can actually subset args.data[i] to produce only the data points we need? This d3 call will produce extra circles for every data point even if it is not visible, which will add a bunch of weight to the DOM that is probably not needed. We are already committing this sin in MG, so I'm hoping to keep us from bringing in an even heavier DOM down the line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You right. It's a bit dirty :/. I will try to fix that this weekend.
The use case is the same than my 2 other PRs => https://twitter.com/champagnethomas/status/972161893384228866

Copy link
Collaborator

Choose a reason for hiding this comment

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

Great - thanks @thomaschampagne!

Copy link
Collaborator

Choose a reason for hiding this comment

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

btw, really neat to see how you're using MG. Don't hesitate to send along more screenshots.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@hamilton I can suggest that improvement:

      if (args.active_point_on_lines) {
        svg.selectAll('circle-' + line_id)
          .data(args.data[i])
          .enter()
          .filter((d) => {
            return d[args.active_point_accessor];
          })
          .append('circle')
          .attr('class', 'mg-area' + (line_id) + '-color mg-shown-active-point')
          .attr('cx', args.scalefns.xf)
          .attr('cy', args.scalefns.yf)
          .attr('r', (d) => {
            console.warn(d)
            return args.active_point_size;
          });
      }

I introduced a filter() between enter() and append('circle'). With this code only active points will create 'circle' elements in the DOM . Tested and verified on my side.

Let me know if this solution is sufficient for you.

Sending more screenshot of my app :)? Or screenshots related to this PR?

@thomaschampagne
Copy link
Contributor Author

@hamilton @wlach Does my latests changes (82bc917) are OK for you? Or i have to review my work?

@wlach
Copy link
Collaborator

wlach commented Mar 29, 2018

Sorry for the delay on reviewing this. Everything looks good to me here, I will merge!

@wlach wlach merged commit b44ec52 into metricsgraphics:master Mar 29, 2018
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.

None yet

3 participants