-
-
Notifications
You must be signed in to change notification settings - Fork 114
Add placeholder value to titles #222
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
Conversation
| const {onChange, localize: _} = this.props; | ||
| let {placeholder, value} = this.props; | ||
|
|
||
| if (value.startsWith('Click to enter') && value.endsWith('title')) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or @alexcjohnson would you know of a better way for me to get that default string when title's not set?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mmm, that way isn't going to work with i18n anyway. Is it possible to compare the value from fullLayout with the value from layout? If they're different, I think that's a robust signal that you have the placeholder. And if you explicitly typed in the placeholder, then it's not really a placeholder anymore :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we're not comfortable using the plotly.js placeholder verbatim, I think we'd have to explicitly make and localize a new placeholder for each attribute that uses this. Doesn't seem worth it to me, frankly; I'd either use the one from plotly.js or use a standard placeholder for every text entry box.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, we could use those plotly.js strings verbatim, it's easier for localization too, like you mention, so I'll put those strings as placeholders then, not values
|
ready for review @alexcjohnson @bpostlethwaite |
20e7461 to
6c69983
Compare
|
container (equivalent of layout) is giving me that attribute's immediate container, so if I'm in xaxis, it's that xaxis object, not the full gd.layout object. and fullValue is taken from gd._fullValue so this seems to work ok to me |
|
oups, I forgot to also check against attr |
|
Great! I'm happy with this, thanks for iterating through it with me. 💃 |
|
thanks! |

closes: #217
