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 fallback for missing font option in canvas/text #6

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

johannhof
Copy link

When using react-art it's hard to understand how text is supposed to work, especially considering that

<Text x={10} y={10}>Hello World!</Text>

renders in SVG mode but not in canvas mode.

This is because in canvas mode, without specifying font={fontSize:12}, lineHeight in renderShapeTo turns to NaN and takes the y parameter down with it.

Considering the dramatic lack of documentation for this otherwise excellent library we should add a fallback so that people don't get demotivated and the render behaviour stays consistent.

@sophiebits
Copy link

Does SVG inherit font from the surrounding context or what?

@johannhof
Copy link
Author

I don't think so but the point is rather that we produce a NaN in the position calculation for the y field without this change, and unsurprisingly, a position of NaN is not handled very gracefully by the canvas.

@johannhof
Copy link
Author

I'm talking about this part

        var em = this._fontSize,
            y = em / 2,
            lineHeight = 1.1 * em,
            lines = text,
            l = lines.length;

        if (fill){
            context.fillStyle = fill;
            for (var i = 0; i < l; i++)
                context.fillText(lines[i], 0, y + i * lineHeight);
        }

@sophiebits
Copy link

Right. Ideally we can pick some default behavior that's consistent across the SVG and Canvas (and VML) implementations.

@johannhof
Copy link
Author

Ah, I see your point. I'll try to find out what the current default behaviour for SVG and VML is.

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