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

Add tooltip attribute to description widgets #1864 #2070

Merged
merged 5 commits into from Jul 6, 2018

Conversation

Projects
None yet
4 participants
@piyushrungta25
Contributor

piyushrungta25 commented May 10, 2018

for #1864

@@ -65,14 +67,19 @@ class DescriptionView extends DOMWidgetView {
updateDescription() {
let description = this.model.get('description');
let tooltip = this.model.get('tooltip');
if (tooltip.length === 0) {
tooltip = description;

This comment has been minimized.

@jasongrout

jasongrout May 10, 2018

Member

How would we disable the tooltip if we didn't want it to show up?

This comment has been minimized.

@piyushrungta25

piyushrungta25 May 10, 2018

Contributor

As far as I understand, it couldn't be disabled completely previously also since it always defaults to the description. Not specifying the tooltip or setting it to an empty string will result in closest behavior.

@jasongrout

This comment has been minimized.

Member

jasongrout commented May 10, 2018

Thanks! One design comment about disabling the tooltip if we'd like. Setting the tooltip to '' seems natural to disable it - perhaps None should be the default, which means take it from the description?

I agree that this change is strictly better than what we have (we can't turn off tooltips right now either).

@jasongrout

This comment has been minimized.

Member

jasongrout commented May 10, 2018

Also, can we call it description_tooltip? An arbitrary control might have other places where tooltips make sense, so scoping the name to where it is used makes sense, even if it is a bit long. We do have precedence for a name like that, in that the style has a description_width attribute.

@piyushrungta25

This comment has been minimized.

Contributor

piyushrungta25 commented May 10, 2018

Thanks for the feedback.

Setting the tooltip to '' seems natural to disable it - perhaps None should be the default, which means take it from the description?

Just so that I understand correctly, the new behavior should be,

  • tooltip = None, use description
  • tooltip = '' , disable tooltip

with None as the default value.

@jasongrout

This comment has been minimized.

Member

jasongrout commented May 10, 2018

Does that make sense to you as a user? It's a bit awkward that we use "None" as a default value, but it is a convention. The way I see it, if you have a string, set the tooltip to that (including ''). If you have None, do the default behavior.

@maartenbreddels

This comment has been minimized.

Member

maartenbreddels commented May 11, 2018

Set 'None' as default for description_tooltip
If widget.description_tooltip is set to a string(including empty string), the
tooltip is set to that. If it is set as None, use description string as default
value.
@piyushrungta25

This comment has been minimized.

Contributor

piyushrungta25 commented May 11, 2018

That does make sense. I made the discussed changes.

@jasongrout

This comment has been minimized.

Member

jasongrout commented Jul 6, 2018

Thanks. We'll need to update the spec to have this trait, then I think we are good to merge. I'll look at this hopefully tomorrow.

@jasongrout jasongrout modified the milestones: Minor release, 7.3 Jul 6, 2018

@jasongrout

This comment has been minimized.

Member

jasongrout commented Jul 6, 2018

@SylvainCorlay - what are your thoughts about the model change here? In particular, the attribute can be either null or a string - does that make things difficult for xwidgets?

@SylvainCorlay

This comment has been minimized.

Member

SylvainCorlay commented Jul 6, 2018

Hey @jasongrout thanks for the heads up.

The model changes are fine with me. We handle optional widget attributes very well in xwidgets.

For example, in the layout widget, we do:

XPROPERTY(xtl::xoptional<std::string>, derived_type, align_items, {}, XEITHER_OPTIONAL("flex-start", "flex-end", "center", "baseline", "stretch", "inherit", "inital", "unset"));
@jasongrout

This comment has been minimized.

Member

jasongrout commented Jul 6, 2018

Great, then I'm okay with merging this for the beta.

@jasongrout jasongrout merged commit c8b6be6 into jupyter-widgets:master Jul 6, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jasongrout

This comment has been minimized.

Member

jasongrout commented Jul 6, 2018

Thanks @piyushrungta25 for pushing this through!

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