Skip to content

enables loading locales before Plotly #4453

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.

Sign up for GitHub

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

Merged
merged 5 commits into from
Jan 2, 2020
Merged

enables loading locales before Plotly #4453

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
6940aac
enable loading locales before Plotly
antoinerg Dec 30, 2019
f169380
delete window.PlotlyLocales after locales are registered
antoinerg Dec 30, 2019
b4b20a7
locales: stash under window.PlotlyConfig.locales instead of window.Pl…
antoinerg Jan 2, 2020
c19fbf4
Revert "locales: stash under window.PlotlyConfig.locales instead of w…
antoinerg Jan 2, 2020
00aadd7
Merge branch 'master' into script-order
antoinerg Jan 2, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions src/core.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

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?

}

// plot icons
exports.Icons = require('./fonts/ploticon');

Expand Down
4 changes: 2 additions & 2 deletions tasks/util/wrap_locale.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,8 @@ var intoStream = require('into-stream');

var constants = require('./constants');

var prefix = 'Plotly.register(';
var suffix = ');';
var prefix = 'var locale=';
var suffix = ';if(typeof Plotly === \'undefined\') {window.PlotlyLocales = window.PlotlyLocales || []; window.PlotlyLocales.push(locale);} else {Plotly.register(locale);}';

var moduleMarker = 'module.exports = ';

Expand Down