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.
| 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?
There was a problem hiding this comment.
Sure, maybe it could log both, I'll update.
| // 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.
There was a problem hiding this comment.
That's a good idea, thanks, I'll do that.
There was a problem hiding this comment.
Oh wait, no, that can't be done directly because of the localization needing a reference to gd. I might be able to structure this a bit more clearly though.
5bc58da to
4897de4
Compare
Co-authored-by: Cameron DeCoster <cameron.decoster@gmail.com>
4 participants
Codepen demo: https://codepen.io/emilykl-code/pen/PwWKqej
Dialog
Button tooltip