Skip to content
This repository has been archived by the owner on Feb 19, 2022. It is now read-only.

Add support for inlining multiple text and passing an array via the lineHeight prop. Features for #893, #902, and #904. #328

Merged

Conversation

parkerziegler
Copy link
Contributor

@parkerziegler parkerziegler commented Jan 16, 2018

This PR accomplishes three tasks, related to issues #893, #902, and #904.

Issue #893: Allow VictoryLabel to render multiple text inline

The first portion of this PR adds support for a boolean inline prop, which specifies that labels passed as an array in VictoryLabel's text prop should be rendered inline. To do this, we simply return undefined for the x and dy attributes if inline is true. This allows the dx prop to specify the relative spacing of inlined <tspan /> elements. For example, this code:

import React from "react";
import { VictoryLabel } from "../src/index";
import data from "../src/victory-util/data";

export default class App extends React.Component {
  render() {

    const example = ["hello", "Formidable", "Labs"];

    return (
      <div className="demo">
        <p>
          VictoryLabel demo! The little circles show the anchor points for
          each label.
        </p>
        <svg width="600" height="1800" style={{ border: "1px solid #ccc", padding: 40 }}>

          <circle cx="300" cy="0" r="2" fill="red"/>
          <VictoryLabel x={300} y={0} textAnchor="end" verticalAnchor="middle"
            text={"Victory is awesome.\nThis is inline styling for <tspan>. Woohoo!"} inline
          />

          <circle cx="300" cy="150" r="2" fill="red"/>
          <VictoryLabel x={300} y={150} textAnchor="start" verticalAnchor="middle"
            text={[ '% of target', '44% for 2017' ]}
            style={[{ fill: '#000' }, { fill: '#6128ff' }]} inline dx={25}
          />

          <circle cx="300" cy="300" r="2" fill="red"/>
          <VictoryLabel x={300} y={300} textAnchor="start" verticalAnchor="start"
            text={example} inline dx={"10"}
          />
        </svg>
      </div>
    );
  }
}

will render:
screen shot 2018-01-16 at 10 56 14 am

We could also consider doing some more explicit styling for inlined labels – this solution just takes advantage of svg's default inline display for <tspan />s inside of <text />.

Issue #902: For a multiline VictoryLabel, I would like to have different lineHeights per line.

The second portion of this PR adds support for passing a number[] or string[] as a valid lineHeight prop. To do this, we check if lineHeight is passed as an array and, if so, average the lineHeight of the current and previous index (similar to how we average fontSize) in order to calculate the proper dy attribute for each <tspan />. For example, the following code:

<circle cx="0" cy="2000" r="2" fill="red"/>
<VictoryLabel
         x={0} y={2000}
         text={["Victory is awesome.", "We can even specify", "a lineHeight array!", "Just like this!"]}
         style={[{ fontSize: 50, fill: "green" }, { fontSize: 60 }, { fontSize: 30 }, { fontSize: 30 }]}
         lineHeight={[1.22, 2, 3, 1]}
        verticalAnchor="start"
/>

will render:
screen shot 2018-01-16 at 4 37 35 pm

We may want to revisit how this interacts with verticalTextAnchor – we use the first lineHeight, similar to how we use the first fontSize, to determine the dy attribute of the <text /> container itself.

Issue #904 Empty arrays for VictoryLabel's style prop cause an error

There are now checks for a user passing empty arrays into both the style and lineHeight props. If an empty array gets passed into style, we default to defaultStyles. If an empty array gets passed into lineHeight, we default to using 1 as the lineHeight.

@boygirl
Copy link
Contributor

boygirl commented Jan 16, 2018

@parkerziegler this looks good to me. Can you add your example to the demo page?

@parkerziegler
Copy link
Contributor Author

@boygirl Added the examples to the bottom of victory-label-demo.js.

Copy link
Contributor

@david-davidson david-davidson left a comment

Choose a reason for hiding this comment

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

Nice! 👏

@parkerziegler parkerziegler changed the title Add support for inlining multiple text. Feature for #893. Add support for inlining multiple text and passing an array via the lineHeight prop. Features for #893 and #902. Jan 17, 2018
@parkerziegler
Copy link
Contributor Author

@boygirl @david-davidson Just updated the main comment for the PR. Should fix #893 and #902 together. Let me know if there are any issues. Both features have examples in the demo app.

@@ -264,10 +267,14 @@ export default class VictoryLabel extends React.Component {
const style = this.style[i] || this.style[0];
const lastStyle = this.style[i - 1] || this.style[0];
const fontSize = (style.fontSize + lastStyle.fontSize) / 2;
const lineHeight = Array.isArray(this.lineHeight)
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to detect/handle empty arrays? (Same question for existing style var too)

Copy link
Contributor

Choose a reason for hiding this comment

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

I suppose for completeness sake we should, but in both of those cases, that would only happen if a user explicitly added the prop like lineHeight={[]}. That would technically be allowed by the prop definition, but doesn't make any intuitive sense. I think it's a highly unlikely edge case to hit, but I agree that it's bad practice to trust the user to not break things.

Copy link
Contributor

Choose a reason for hiding this comment

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

@parkerziegler can you add a fallback for empty arrays for lineHeight in this PR. I'll create a new issue for styles

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@boygirl @ryan-roemer Sure thing, I'll do it first thing when I get in tomorrow.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ryan-roemer I think I'm pretty much all set to push the changes. We fallback to default values for both style and lineHeight if empty arrays are passed for those props. Should I add a console.warn just to let users know this is not a recommended practice?

@boygirl
Copy link
Contributor

boygirl commented Jan 17, 2018

@parkerziegler code looks good, but please lint the changes you added to the demo

@parkerziegler
Copy link
Contributor Author

@boygirl This should be updated. I added 4 test cases to cover the issues in victory-label.spec.js. They all pass, and the linting passes as well. For reference:
formidable-victory-label-tests.

@parkerziegler parkerziegler changed the title Add support for inlining multiple text and passing an array via the lineHeight prop. Features for #893 and #902. Add support for inlining multiple text and passing an array via the lineHeight prop. Features for #893, #902, and #904. Jan 18, 2018
@boygirl boygirl merged commit 73534d4 into FormidableLabs:master Jan 18, 2018
@parkerziegler parkerziegler deleted the feature/inline-multi-label branch January 18, 2018 22:13
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants