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

enables loading locales before Plotly #4453

Merged
merged 5 commits into from
Jan 2, 2020
Merged

enables loading locales before Plotly #4453

merged 5 commits into from
Jan 2, 2020

Conversation

antoinerg
Copy link
Contributor

This PR enables loading locales before plotly.js is on the page and partially fixes #4146.

In the following Codepens, we expect months to be in French (it should be Mai instead of May):
Before
After

src/core.js Outdated
@@ -65,6 +65,11 @@ register([
require('./locale-en-us')
]);

// locales that are present in the window should be loaded
if(window.PlotlyLocales && window.PlotlyLocales.length) {
Copy link
Contributor

Choose a reason for hiding this comment

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

if(window.PlotlyLocales && Array.isArray(window.PlotlyLocales)) {
// ...
}

would be safer. Note that Plotly.register([]) is handled correctly (essentially a noop) at the moment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in f169380

@etpinard
Copy link
Contributor

This is an interesting solution. I'm not a fan of us polluting the window namespace once more with this new PlotlyLocales global, but I can't really think of a better solution, so 👌

@@ -65,6 +65,11 @@ register([
require('./locale-en-us')
]);

// locales that are present in the window should be loaded
if(window.PlotlyLocales && window.PlotlyLocales.length) {
register(window.PlotlyLocales);
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we could after this line have:

delete window.PlotlyLocales;

to de-pollute the window namespaces after we're done registering the locales.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in f169380

Copy link
Contributor

Choose a reason for hiding this comment

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

Since it is a temporary variable, it may be better to add an underscore i.e. window._PlotlyLocales.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the suggestion! I'll let @etpinard choose between the different options (see also #4453 (comment))

Copy link
Contributor

Choose a reason for hiding this comment

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

Wondering using sessionStorage may be a possibility?

@antoinerg
Copy link
Contributor Author

This is an interesting solution. I'm not a fan of us polluting the window namespace once more with this new PlotlyLocales global, but I can't really think of a better solution, so

@etpinard Would you prefer me using a locales key in the window.PlotlyConfig object. I know it is used to configure MathJax here.

@etpinard
Copy link
Contributor

etpinard commented Jan 2, 2020

Would you prefer me using a locales key in the window.PlotlyConfig object. I know it is used to configure MathJax here.

I forgot about PlotlyConfig. Thanks for pointing this out! Yes, I'd vote for putting your new locales stash in PlotlyConfig.

@alexcjohnson
Copy link
Collaborator

Only issue with using PlotlyConfig is if a user includes window.PlotlyConfig = {MathJaxConfig: 'local'} - which is the documented way to support parallel MathJax usage - we'll still have some order-dependent behavior because that will wipe out the locales if they're loaded before the PlotlyConfig script and that script is loaded before plotly.js. I think it'd be better to keep PlotlyConfig reserved for explicitly user-provided behavior. I think window.PlotlyLocales is fine. That said, if we want to be really clever (and hard to debug...) we might even be able to use window.Plotly, as this won't exist yet during the execution of the plotly.js script, right?

@antoinerg
Copy link
Contributor Author

That's a very good point @alexcjohnson. I reverted b4b20a7 to use window.PlotlyLocales. Should we underscore it or is it OK as it is?

@alexcjohnson
Copy link
Collaborator

I'm OK with window.PlotlyLocales, unless anyone feels strongly about the underscore.

@etpinard
Copy link
Contributor

etpinard commented Jan 2, 2020

I'm OK with window.PlotlyLocales, unless anyone feels strongly about the underscore.

Yeah, that's fine 👌


@antoinerg can you merge the latest master into your branch to get the syntax (header) tests to pass?

@etpinard
Copy link
Contributor

etpinard commented Jan 2, 2020

Nicely done @antoinerg - 💃 !

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

Successfully merging this pull request may close these issues.

Order-independent script tags (MathJax & locales)
4 participants