Update "Share chart" dialog wording - #7928
Conversation
… default (Plotly Cloud) URL vs. ccustom URL provided in config
|
LGTM! |
|
Should this be sentence case? @red-patience |
If this sets a new precedent then I would recommend sentence case but if not, then consistency with what exists already. |
Which part? |
|
"Share chart" instead of current "Share Chart" |
emilykl
changed the title
|
@red-patience @ndrezn I don't feel strongly either way, and there's not much precedent in plotly.js itself, so I've lowercased the word "chart" both in the button tooltip and the dialog title. |
camdecoster
left a comment
camdecoster
left a comment
There was a problem hiding this comment.
I left a few comments, but no blockers.
| @@ -0,0 +1 @@ | |||
| - Update "Share Chart" dialog with more informative wording [[#7928](https://github.com/plotly/plotly.js/pull/7928)] No newline at end of file | |||
There was a problem hiding this comment.
| - Update "Share Chart" dialog with more informative wording [[#7928](https://github.com/plotly/plotly.js/pull/7928)] | |
| - Update "Share chart" dialog with more informative wording [[#7928](https://github.com/plotly/plotly.js/pull/7928)] |
| return; | ||
| } | ||
| if (baseUrlObj.protocol !== 'https:' && baseUrlObj.protocol !== 'http:') { | ||
| console.error('Invalid protocol for plotlyServerURL: ' + baseUrl); |
There was a problem hiding this comment.
Should this log the invalid protocol rather than the URL?
|
|
||
| var _ = require('../../lib')._; | ||
| const _ = require('../../lib')._; | ||
| const dfltConfig = require('../../plot_api/plot_config').dfltConfig; |
There was a problem hiding this comment.
| const dfltConfig = require('../../plot_api/plot_config').dfltConfig; | |
| const { dfltConfig } = require('../../plot_api/plot_config'); |
| // Wording for dialog box. Must be defined inside this function rather than | ||
| // at the top of the file because localization requires a reference to the | ||
| // graph div (gd) |
There was a problem hiding this comment.
Could these strings be defined in a constants.js file? I've seen that pattern in the repo.
4 participants
Codepen demo: https://codepen.io/emilykl-code/pen/PwWKqej
Dialog
Button tooltip