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

Update wrapText to handle multiple units #248

Open
katiepark opened this issue Oct 6, 2017 · 2 comments
Open

Update wrapText to handle multiple units #248

katiepark opened this issue Oct 6, 2017 · 2 comments
Assignees

Comments

@katiepark
Copy link
Contributor

The wrapText helper function assumes the selector's dx and dy values are in pixels, and assigns the new computed values in pixels: https://github.com/nprapps/dailygraphics/blob/master/graphic_templates/_base/js/helpers.js#L99-L124

This can be an issue when calling wrapText on text elements generated by d3 (such as an axis tick), because d3 often sets these values in ems. The solution should either detect and use the selector's units or should use a computed value in pixels (not totally sure if this can be done with dy properties).

This is pretty related to #247, which should probably be done first.

@katiepark
Copy link
Contributor Author

Some initial thoughts on how this might work:

  1. Check whether the dx/dy attribute contains a unit.
    a. Potential approach: use regex (maybe overkill)
    b. Potential approach: check whether the attribute is equal to the numeric representation of the attribute.
  2. Store the unit as a variable. If it doesn't contain a unit, default to pixels.
  3. Do any calculations on the attribute using the numeric representation of the attribute
  4. Append the unit to the calculated value when setting the attribute value in d3

Under this approach, line height would have to be in the same units as dy. But that doesn't feel like that's too much to ask.

We'd still likely have to limit the accepted units. So far I have really only used pixels and ems; I guess someone could potentially want to use percent but I don't think we have to get that fancy.

@katiepark
Copy link
Contributor Author

Here's a first stab at this fix: a33e521 It uses approach 1b (above) for checking the unit.

Also imperfect because line height must be passed as a number and it must match the units of the dy attribute (which is always a string or undefined).

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

No branches or pull requests

1 participant