Skip to content

Button Icons/FontAwesome regression no longer supports fa-spin #2477

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

Closed
rskabelund opened this issue Jul 3, 2019 · 18 comments · Fixed by #2685
Closed

Button Icons/FontAwesome regression no longer supports fa-spin #2477

rskabelund opened this issue Jul 3, 2019 · 18 comments · Fixed by #2685
Labels
good first issue resolved-locked Closed issues are locked after 30 days inactivity. Please open a new issue for related discussion.
Milestone

Comments

@rskabelund
Copy link

rskabelund commented Jul 3, 2019

since this was added:
https://github.com/jupyter-widgets/ipywidgets/blob/master/ipywidgets/widgets/widget_button.py#L65-L73

we are no longer able to pass in additional fa classes to specify fa-spin or fa-lg etc.

I really liked my spinning gear icon button before this change...

i.e.

from ipywidgets import Button
Button(description='Running...', icon='fa-gear fa-spin fa-lg')

it will error on the add of the DOMTokenList due to the spaces in the string
https://github.com/jupyter-widgets/ipywidgets/blob/master/packages/controls/src/widget_button.ts#L91

@rskabelund
Copy link
Author

rskabelund commented Jul 3, 2019

Running on ipywidgets 5.2.2
fontawesomebtndisabled

@rskabelund
Copy link
Author

@jasongrout what would be the path forward to allow multiple font awesome parameters? Are there existing accommodations for this that I am missing?

@kebwi
Copy link

kebwi commented Sep 11, 2019

I was just wishing yesterday for multiple icons on a button and found this discussion. The implication is that it used to work and such behavior was explicitly removed? Why not leave it in and let the user decide if they find multiple icons useful? I could use it frankly.

Thanks.

@rskabelund
Copy link
Author

@kebwi I ended up creating a custom widget for my purposes using the ipywidgets cookiecutter template. But, yeah, disappointed this support is no longer available with little explanation why.

@rskabelund
Copy link
Author

rskabelund commented Sep 11, 2019

Here is an example custom font awesome button:

Python Model:

from traitlets import Unicode, Bool, validate, TraitError, Any
from ipywidgets import DOMWidget, register

@register
class FaButton(DOMWidget):
    _view_name = Unicode('FaButtonView').tag(sync=True)
    _view_module = Unicode('fabutton_widget').tag(sync=True)
    _view_module_version = Unicode('0.1.0').tag(sync=True)

    # Attributes
    description = Unicode('ww', help="Button Text").tag(sync=True)
    icon = Unicode('fa-cloud-download', help="Font Awesome String").tag(sync=True)
    disabled = Bool(False, help="Enable or disable button").tag(sync=True)
    callback_event = Any().tag(sync=True)

Javascript View:

%%javascript
require.undef('fabutton_widget');

define('fabutton_widget', ["@jupyter-widgets/base"], function(widgets) {

    var FaButtonView = widgets.DOMWidgetView.extend({

        // Render the view.
        render: function() {
            this.btn = document.createElement('button');            
            this.btn.setAttribute("class", "jupyter-button widget-button");
            
            this.update_text();
            
            this.el.appendChild(this.btn);
            
            this.el.setAttribute("class", "widget-html-content");
            
            // Python -> JavaScript update
            this.model.on('change:icon', this.update_text, this);
            this.model.on('change:description', this.update_text, this);

        },

        events: {
            // List of events and their handlers.
            'click': 'handle_click'
        },   
        
        handle_click: function() {
            if (event.target.tagName == 'BUTTON') {
                this.model.set('callback_event', {'target':event.target.innerHTML, 'data':event.target.dataset, 'type': 'button_click' , 'name':event.originalEvent , 'timeStamp':event['timeStamp']}); 
                this.touch();
            }
        },

        update_text: function() {
            this.btn.innerHTML = '<i class="fa ' + this.model.get('icon') + '" aria-hidden="true"></i>' + this.model.get('description');
        },

    });

    return {
        FaButtonView: FaButtonView
    };
});

Usage:

def handle_btn_click_callback(e):
    print('hello from custom btn widget')
    
    
btn = FaButton(icon='fa-spin fa-gear', description = 'Hello')
btn.observe(handle_btn_click_callback, 'callback_event')
display(btn)

@kebwi
Copy link

kebwi commented Sep 12, 2019

Hmmm. Thanks. I admit, I've haven't delved into custom widgets yet. I guess it's time to roll up my sleeves.

@vidartf
Copy link
Member

vidartf commented Sep 12, 2019

I did a bit of digging in the code, and I think this is where this behavior was changed: #427 (after this it was no longer possible to define multiple icon classes). That is three years ago, although the deprecation warning was added a few months later. The purpose of that was to reduce/remove our dependency on jQuery.

It might be possible to get accepted a PR that re-adds support for multiple classes. Change this:

i.classList.add('fa-' + icon);

To this:

i.className += 'fa-' + icon;

You would then be able to use (the slightly hackish):

from ipywidgets import Button
Button(description='Running...', icon='gear fa-spin fa-lg')

@jasongrout
Copy link
Member

jasongrout commented Sep 12, 2019

Perhaps something like:

icon.split(/[\s]+/).filter(Boolean).map(v => `fa-${v}`).join(' ')

would let the icon take multiple values.

@vidartf
Copy link
Member

vidartf commented Sep 12, 2019

Note: The change above only outlines how to support multiple icon classes. The other change that was done was to automatically prefix with fa-. That seems to have been done (by mistake?) in #555 . Since this has been in for such a long time now, I don't think that can be considered a non-breaking fix anymore.

@jasongrout
Copy link
Member

This may be a simpler way to prepend the fa- to each word:

icon.replace(/\b(\w)/g, 'fa-$1')

@jasongrout
Copy link
Member

The other change that was done was to automatically prefix with fa-. That seems to have been done (by mistake?) in #555 . Since this has been in for such a long time now, I don't think that can be considered a non-breaking fix anymore.

That was introduced in 6.0, apparently (at least #555 was in 6.0).

@jasongrout jasongrout added this to the Minor release milestone Sep 12, 2019
@jasongrout
Copy link
Member

I think it would be great if someone put in a PR adding back in support for multiple icons, probably by just changing the one line that Vidar points out above (I give several different ways to prepend fa- to each thing in the list above).

Setting as a good first issue.

@vidartf
Copy link
Member

vidartf commented Sep 12, 2019

That was introduced in 6.0

Yes, which is why I think we simply said "this is the new behavior now" instead of saying "this is a bug, lets fix it". 🤷‍♂

@kebwi
Copy link

kebwi commented Sep 12, 2019

Thanks for the encouragement. It would be great to download and otherwise contribute to ipywidgets as opposed to just using it. But I admit, I'm not sure I have time to ramp up on the code or otherwise become a quality contributor right now. I'll weigh my priorities.

Cheers!

@jasongrout
Copy link
Member

I'm hoping that this fix is literally just changing

i.classList.add('fa-' + icon);
to

i.classList.add(...icon.split(/[\s]+/).filter(Boolean).map(v => `fa-${v}`)); 

@jasongrout
Copy link
Member

And maybe changing the documentation at

font-awesome icon name
and
icon = Unicode('', help="Font-awesome icon name, without the 'fa-' prefix.").tag(sync=True)

@martinRenou
Copy link
Member

I'll try to have a look at this issue tomorrow

@martinRenou
Copy link
Member

martinRenou commented Jan 8, 2020

PR opened #2685

@kebwi Please note that this PR does not add support for multiple icons in a Button. It is only fixing the regression that appeared in 6.0, allowing:

  • spinning icons
  • larger icons

Which I understand is the original issue here. Maybe we could open another issue for multiple icons support?

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

Successfully merging a pull request may close this issue.

5 participants