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

Rtl regresssions in v3 #9194

Closed
danielgindi opened this issue May 31, 2021 · 3 comments · Fixed by #9199
Closed

Rtl regresssions in v3 #9194

danielgindi opened this issue May 31, 2021 · 3 comments · Fixed by #9199

Comments

@danielgindi
Copy link
Contributor

danielgindi commented May 31, 2021

Expected Behavior

rtl: true in legend should draw it rtl (box to the right of the text) like in charts v2.

Current Behavior

text goes off screen, not rendered at all.

Possible Solution

I have no idea. The refactor to v3 completely changed (ruined?) the original rtl helper that I've written.
There are major regressions, like renderText rendering multiples lines always aligned to the left, while it should be alignment-aware.
And everything else has changed.

Looks like _textX should be updated to be rtl-aware.

Steps to Reproduce

https://codepen.io/danielgindi/pen/eYveogd

Context

Cannot upgrade to v3 due to rtl issues.

Environment

  • Chart.js version: 3.3.2
  • Chrome 91.0.4472.77
@etimberg etimberg added this to the Version 3.4.0 milestone May 31, 2021
@etimberg
Copy link
Member

@danielgindi during v3 development, I do not recall changing the rtl adapters. They are still used in both the legend and tooltip plugins. It looks like there is one line where the textAlign is not translated. https://github.com/chartjs/Chart.js/blob/master/src/plugins/plugin.legend.js#L374

I think this should become

textAlign: rtlHelper.textAlign(legendItem.textAlign)

As an immediate work-around, you can override the generateLabels callback to return the correct alignment. Example: https://codepen.io/etimberg/pen/rNyYggP

@danielgindi
Copy link
Contributor Author

@etimberg well there is a new _textX function which is not rtl aware on itself (but may expect rtl-normalized arguments?)

Also I was mistaken about renderText and the lines, as the canvas context automatically starts from the right based on the alignment passed so the arguments need to be rtl-ed, not the function itself.
Which means you may be right and the textAlign is the whole issue here :-)

Your fix seems to work fine!

I'll wait for it to be fixed on master so I don't have regressions when updating.

@etimberg
Copy link
Member

I quickly tested my fix, but it broke two tests when the textAlign is directly set in the options. I think you are correct that _textX will need to be updated as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants