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

Radar chart tooltips #992

Merged
merged 7 commits into from
Oct 15, 2018
Merged

Conversation

emilysallstrom
Copy link
Contributor

@emilysallstrom emilysallstrom commented Oct 4, 2018

This is a pull request for the issue I logged recently: #959

This has a few changes:

  1. RadarChart - add the option to display axes on top of the polygons. I have charts with high opacity that cover the axes labels. I added a prop to give the option to render the axes on top (false by default). (The 1, 2, 3, 4 labels in the screenshot below illustrate this.)

  2. CustomSvgSeries - ]add onMouseOver/onMouseOut events

  • Showcase (and tests) updated to show an example:

screen shot 2018-10-09 at 1 18 17 pm

  1. RadarChart - Tooltips on the Polygon corners.
  • Added onValueMouseOver and onValueMouseOut
  • Circle rendered transparently with MarkSeries - has a size of 14 so that the mouse just needs to be close to the point to show the tooltip.
  • This screenshot is an example that has been added to the showcase. New tests have also been added.

screen shot 2018-10-09 at 1 19 48 pm

  1. RadarChart - onSeriesMouseOver/onSeriesMouseOut events:

screen shot 2018-10-09 at 1 19 59 pm

@CLAassistant
Copy link

CLAassistant commented Oct 4, 2018

CLA assistant check
All committers have signed the CLA.

@mcnuttandrew mcnuttandrew self-requested a review October 5, 2018 00:27
Copy link
Contributor

@mcnuttandrew mcnuttandrew left a comment

Choose a reason for hiding this comment

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

Hey @emilyjeppson,

This is great! Thanks for doing this! I liked that you gave mouse overs to the custom SVG in doing so.

src/plot/series/custom-svg-series.js Outdated Show resolved Hide resolved
src/radar-chart/index.js Outdated Show resolved Hide resolved
import RadarChart from 'radar-chart';
import {Hint} from 'index';

const DATA = [
Copy link
Contributor

Choose a reason for hiding this comment

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

I think your example will stand out more if these series don't all have the same fils

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So the reason I did it this way is because I wanted to closely mimic my implementation, which uses the first 5 sets of data to draw polygon-shaped gridlines. (Similar to CircularGridLines, but with straight lines instead of round.) This makes it look more like a traditional "Spider" radar chart, which I thought could be useful for others trying to implement a radar chart? I did change them to alternating gray/white, but I could also completely remove them if you think that's better.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh i see! In that case, i think this is probably fine for now if you leave a comment in the code about what this is the way it is. In the future it would be good to add a new component just for the radar chart to produce those spider grids (in fact would you mind creating an issue to that effect?).

src/radar-chart/index.js Outdated Show resolved Hide resolved
src/radar-chart/index.js Outdated Show resolved Hide resolved
};
});

return (
Copy link
Contributor

Choose a reason for hiding this comment

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

so instead of mapping all of your points to customSVGSeries here you'd would just map them to an array and then use that as the input to a mark series or voronoi

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I looked into the voronoi for a little bit, and I agree it would be cool to add here (but I didn't add it)

Copy link
Contributor

Choose a reason for hiding this comment

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

Fair this PR has already gotten quite large

@mcnuttandrew
Copy link
Contributor

Oh I was trying to find this pr (#771) yesterday, this resolves that

@emilysallstrom
Copy link
Contributor Author

Thanks @mcnuttandrew! I think this is ready for review again. I update the description to more closely align with the changes.

Copy link
Contributor

@mcnuttandrew mcnuttandrew left a comment

Choose a reason for hiding this comment

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

Oh my goodness, this is a wonderful PR. It covers a lot of different things and adds tests for everything. The last thing it needs before it can be merged in is updates to the docs for the new props you've added.

};
});

return (
Copy link
Contributor

Choose a reason for hiding this comment

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

Fair this PR has already gotten quite large

@@ -75,12 +75,23 @@ test('CustomSVGSeries: Showcase Example - CustomSVGRootLevelComponent', t => {
});

test('CustomSVGSeries: Showcase Example - CustomSVGAllTheMarks', t => {
Copy link
Contributor

Choose a reason for hiding this comment

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

🤩🤩🤩🤩🤩🤩🤩

import RadarChart from 'radar-chart';
import {Hint} from 'index';

const DATA = [
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh i see! In that case, i think this is probably fine for now if you leave a comment in the code about what this is the way it is. In the future it would be good to add a new component just for the radar chart to produce those spider grids (in fact would you mind creating an issue to that effect?).

@mcnuttandrew
Copy link
Contributor

Stamp! Before i merge it in will you resolve the merge conflict that has arisen?

@emilysallstrom
Copy link
Contributor Author

@mcnuttandrew Done! I think I was merging as you were writing your last comment 😄

@mcnuttandrew mcnuttandrew merged commit 6b87bd5 into uber:master Oct 15, 2018
@mcnuttandrew
Copy link
Contributor

MERGED WOOOOOOOOO

@emilysallstrom
Copy link
Contributor Author

WOOO!! Thanks @mcnuttandrew!

@emilysallstrom emilysallstrom deleted the radar-chart-tooltips branch October 15, 2018 22:14
ayarcohaila pushed a commit to ayarcohaila/react-vis that referenced this pull request Jun 30, 2021
* RadarChart: Add ability to show tooltips at the end of each polygon

* Add showcase file for radar chart with tooltips

* Fix eslint errors

* Updates based on review: add onSeriesMouseOver, convert from CustomSVGSeries to MarkSeries

* Fix lint errors

* Added new params to documentation

* Fix merge conflicts
ayarcohaila added a commit to ayarcohaila/react-vis that referenced this pull request May 30, 2023
* RadarChart: Add ability to show tooltips at the end of each polygon

* Add showcase file for radar chart with tooltips

* Fix eslint errors

* Updates based on review: add onSeriesMouseOver, convert from CustomSVGSeries to MarkSeries

* Fix lint errors

* Added new params to documentation

* Fix merge conflicts
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.

3 participants