Review of 6.x changes

[SylvainCorlay]

We spent some time today with @jasongrout reviewing the changes in the 6.x branch (PR #538).

• In utils.js
  • escape.html should use textContent instead of innerHTML
  • typeset should use Array.prototype.map instead of a for loop.
• In lots of places, the double quotes were replaced with single quotes and ' had to be escaped, which highlighted that we should replace couldn't with could not in most error messages.
• In widget_bool.js why is the fa class removed from <i />?
• In widget_box.js, missing quotes around class vbox.
• In the color picker, addEventListener is called twice on the color picker, while in the original widget, it was called once on the picker and once on the text input.
• In widget-int.js we are still using the bootstrap class form-control. SelectionSlider bugfix #529 seems to be fixing it.

[SylvainCorlay]

cc @afshin

[afshin]

@SylvainCorlay @jasongrout,

escape_html should use textContent instead of innerHTML

Just to be clear, which of these behaviors do we want? (Please see attached screenshot.)

It seems clear that we do not want the first or the last. Of the middle two, one escapes the HTML and one strips it. Given that the function is called escape_html, I imagine we want the one with all the HTML character entities in it, but please confirm. The fix below goes with the second choice, but if we decide something else, I will amend it.

(Fix: e72dda5)

typeset should use Array.prototype.map instead of a for loop.

Could you explain the rationale for this? typeset is called in a lot of places:

1. A for loop is faster than map.
2. Perhaps even the for loop is unnecessary: are there actually cases where typeset is returning an array? The reason it currently does is because we were trying to keep a 1:1 fidelity with the jQuery version, but if it's actually the case that individual elements are passed in instead of collections, perhaps we can get rid of the loop altogether.

In lots of places, the double quotes were replaced with single quotes and ' had to be escaped, which highlighted that we should replace 'couldn\'t' with 'could not' in most error messages.

Good call.

(Fix: 0f6a2d9)

In widget_bool.js why is the 'fa' class removed from <i />?

Because it is not necessary. An <i> tag can be given a FontAwesome icon without needing that class. For example, the caret in the DropDownView widget has the following style:

i.widget-caret {
    font-size: 1em;
    font-style: normal;
    font-family: FontAwesome;
    line-height: 1em;
    position: relative;
}

i.widget-caret:before {
    content: "\f0d7";
}

In widget_box.js, missing quotes around class vbox

Bug! Nice find.

(Fix: f81236a)

In the color picker, addEventListener is called twice on the color picker, while in the original widget, it was called once on the picker and once on the text input.

Another bug!

(Fix: 6fb6416)

In widget_int.js we are still using the bootstrap class form-control. #529 seems to be fixing it.

I still see a reference to 'form-control' in my outstanding PR. I have pushed a separate fix in the PR related to this issue.

(Fix: e430f9e)

[jasongrout]

1. escape_html should escape, just like you have it. Thanks
2. Good call - it looks like every call now is just asked to typeset a single element. Let's get rid of the for loop and assume element is a single element - that will simplify the code a lot. Thanks for catching this.
3. Thanks
4. The fa class also sets other things like the font size that correspond to the design of FontAwesome, which I think has ramifications for having pixel-perfect icons, etc (https://github.com/FortAwesome/Font-Awesome/blob/master/css/font-awesome.css#L14). @SylvainCorlay, can you comment? It also makes things forward-compatible with css that FontAwesome may need (because they would change the fa class to reflect changes in the library, presumably)
5. Thanks.
6. Thanks.
7. Thanks.

Thanks again for your work on this!

[SylvainCorlay]

A couple of issues that are master-only:

• The vertical progress bar has the horizontal shape:

(It seems to be filling vertically as expected but has the wrong shape). In 5.x it has the same height as the slider.

• The color picker does not have the standard width anymore.
• The dropdown 's width should be the standard width including the description.

[jasongrout]

@afshin ^

[afshin]

Thanks, @jasongrout and @SylvainCorlay! I'll be looking at these today!

[afshin]

With the new bugfixes:

[SylvainCorlay]

Does it align well with the vertical slider?
On May 3, 2016 2:25 PM, "A. Darian" [email protected] wrote:

With the new bugfixes:
[image: screen shot 2016-05-03 at 7 24 19 pm]
https://cloud.githubusercontent.com/assets/159529/14993860/c3a1809e-1164-11e6-8705-f2cda3f6da9a.png

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#544 (comment)

[afshin]

@SylvainCorlay there might be some tweaks that would make it better. I'll take a look at the CSS:

[afshin]

FYI: https://github.com/ipython/ipywidgets/pull/557/files

[afshin]

FYI, I added the .fa class back in for button icons.