Skip to content
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 support for specifying custom icons in ToggleButtons #2182

Closed
wants to merge 12 commits into from

Conversation

astrofrog
Copy link
Contributor

This adds support for specifying custom icons via the data URI mechanism in ToggleButtons. Here is a preview:

screen shot 2018-08-25 at 16 29 33

A few open questions:

  • One thing I wasn't quite sure about is how the icon size should be handled, and how to make sure the icons are properly centered (in the example above there is a slight vertical offset for the two custom icons) - I'm not familiar enough with the notebook CSS.

  • What is the mechanism for adding tests? I can't see any tests currently for ToggleButtons currently?

  • I can add the same mechanism for ToggleButton once things are polished with ToggleButtons.

Impremented during the JupyterCon 2018 sprint

@jasongrout
Copy link
Member

Thanks!

I've been thinking more about this, and particularly thinking about consistency with the Image widget. What do you think of having a new attribute, icon_format, that can be a mimetype, or the string 'url', or the string 'fontawesome' (defaulting to 'fontawesome'). The icon attribute itself would be changed to binary, and the format string would be used to determine how to interpret the icon attribute.

This would be a backwards incompatible change, and might be overkill, considering icons usually are fairly small, so it's not a big deal to transmit them as data URIs rather than in binary. It does make it slightly easier to give an image, as in you don't have to construct a data uri for the image (you can just read in the binary image and set the attribute to the data). Thoughts? How easy is it to construct data URIs in python?

@maartenbreddels
Copy link
Member

maartenbreddels commented Aug 31, 2018 via email

@jasongrout
Copy link
Member

Do you mean the icon attribute would actually be a reference to an image widget?

@maartenbreddels
Copy link
Member

maartenbreddels commented Aug 31, 2018 via email

@jasongrout
Copy link
Member

I don't think we have precedent for having an attribute that is the union of a widget reference or a string. @SylvainCorlay - do you have thoughts about having a union attribute of either a widget reference or a string? In the serialization, this really means that if the string starts with the magic value, it's considered a widget reference, otherwise it's a fontawesome name.

@maartenbreddels
Copy link
Member

maartenbreddels commented Aug 31, 2018 via email

@SylvainCorlay
Copy link
Member

The icon trait could be of a special Icon type.

Assigning a string would create a font-based icon upon traitlet validation. To get an image, explicitely instantiate an image-based icon.

@maartenbreddels
Copy link
Member

maartenbreddels commented Sep 1, 2018 via email

@SylvainCorlay
Copy link
Member

SylvainCorlay commented Sep 2, 2018

Yes, probably. We also need to do some work on line height for the containing element for vertical centering.

@maartenbreddels maartenbreddels force-pushed the custom-icons branch 2 times, most recently from 4141afb to aaea487 Compare September 25, 2018 14:10
@maartenbreddels
Copy link
Member

I've turned all icon/icons into Icon widgets/List of icon widgets. Icon is basically Image + fontawesome support.
See this demo for features, setting a custom .svg image as icon.
icon

@maartenbreddels
Copy link
Member

I've started supporting non-unique labels with this PR, but it would make sense to do this with all of the _Selection widget. Shall I leave this in this PR and once we agree do another PR to support it in the others as well?

# warnings.warn("icons names no longer start with 'fa-', "
# "just use the class name itself (for example, 'check' instead of 'fa-check')", DeprecationWarning)
# value = value[3:]
# return value
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

leftover?

i.classList.add('fa-' + icon);
if (icon) {
this.iconView = <IconView> await this.create_child_view(icon)
if (description.length === 0) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since of the async we should first put in a placeholder element.

@maartenbreddels
Copy link
Member

@SylvainCorlay issues addressed that we discussed, should we merge?

@maartenbreddels
Copy link
Member

oops, I see I need to follow the same pattern in ToggleButton

@maartenbreddels
Copy link
Member

Ok, this PR led me to find #2256, we should first solve that and then rebase this on master.

@maartenbreddels
Copy link
Member

With #2256 in and some cleanups, this is ready for a new review or merge.

@SylvainCorlay SylvainCorlay added this to the Minor release milestone Jun 4, 2019
@maartenbreddels
Copy link
Member

I think with https://github.com/mariobuikhuizen/ipyvuetify I personally don't need this anymore. Happy for someone else to pick this up.

@lock lock bot added the resolved-locked Closed issues are locked after 30 days inactivity. Please open a new issue for related discussion. label Jun 24, 2020
@lock lock bot locked as resolved and limited conversation to collaborators Jun 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
resolved-locked Closed issues are locked after 30 days inactivity. Please open a new issue for related discussion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants