VictoryCursorContainer #469
Conversation
VictoryCursorContainer
|
|
| static displayName = "VictoryCursorContainer"; | ||
| static propTypes = { | ||
| ...VictoryContainer.propTypes, | ||
| defaultCursorValue: PropTypes.oneOfType([PropTypes.number, PropTypes.object]), |
There was a problem hiding this comment.
Prefer PropTypes.shape(x... y...) to PropTypes.object
| return newElements; | ||
| } | ||
|
|
||
| getTooltip(props) { |
| y: cursorCoordinates.y - 10, | ||
| text: Helpers.evaluateProp(labels, cursorPosition, true), | ||
| active: true, | ||
| key: "cursor-tooltip" |
| }; | ||
| if (labels) { | ||
| newElements.push(React.cloneElement(labelComponent, { | ||
| x: cursorCoordinates.x + 5, |
There was a problem hiding this comment.
hard coded positioning here seems wrong. Maybe a labelOffset prop that can take a single value, or values for x and y.
There was a problem hiding this comment.
The values you have here can be the defaults
There was a problem hiding this comment.
thank you! i felt the same way
| const newElements = []; | ||
| const domainCoordinates = Selection.getDomainCoordinates(scale, domain); | ||
|
|
||
| const cursorCoordinates = { |
There was a problem hiding this comment.
You're already getting the svg coordinates of the mouse position and transforming those into data coordinates in your helpers. You could just add the svg coordinates to the mutation in onMouseMove
There was a problem hiding this comment.
totally understand. i think i'll keep it this way though, because it handles getting the position when defaultCursorValue (which can be both number and object) is used (i.e. there is no onMouseMove).
|
Oh! actually, I just noticed... do |
|
@boygirl YES, you are correct. When We could rename the props. This is definitely the simplest solution, but also is kind of skirting the problem. But hey, being specific (like with I'm not sure exactly what another solution would look like. |
|
In that case, let's go with |
…fset) to avoid collision using createContainer
|
@boygirl addressed those issues. waiting for CI to pass before I merge anything. also, i went with |
|
@chrisbolin looks good to me! Approved! |
|
I'm about to do a quick release, so I'm going to go ahead and merge this :) |
Implements VictoryCursorContainer (FormidableLabs/victory#514)
demo at http://localhost:3000/#/cursor-container
Open issues:
labelsis used (both containers try to create labels, but they are in different places and for different values)