seiyria / bootstrap-slider

A slider control for Bootstrap 3 & 4.
http://seiyria.github.io/bootstrap-slider/
Other
2.99k stars 1.14k forks source link

try getDataAttrib crash app ? #863

Closed jonlepage closed 5 years ago

jonlepage commented 5 years ago

You have strange bug here. HTML code

// create new content for tbody tr colors pickers
function new_HTML_contentSlidersFalloff(targets){
    return [/*html*/ `
        <tr>
            <td>
                <div class="input-group-text"> <p>fallOff:</p> </div>
            </td>
            <td>
                <div class="form-control dark"> <!--light fallOff Slider -->
                <p>[Kc: Constante, Kl: Linéaire Kq: Quadratique]</p>
                    <b>Kc:</b> <input id="Kc" type="text" data-slider-handle="square" class="span2"/><br>
                    <b>Kl:</b> <input id="Kl" type="text" data-slider-handle="square" class="span2"/><br>
                    <b>Kq:</b> <input id="Kq" type="text" data-slider-handle="square" class="span2"/><br>
                </div>
            </td>
            <td>
                <input class="saveCheck" type="checkbox" id=fallOff_select>
            </td>
        </tr>
    `];
};

image

The try crash my app I thin you should replace the TRY by classic conditional ?! What do you think about this?

                if (typeof dataValString === 'string') {
                    return dataValString;
                }else{
                    return JSON.parse(dataValString);
                }

Do you have a better idea? thanks

seiyria commented 5 years ago

You'll need to make a jsfiddle.

jespirit commented 5 years ago

I can't seem to reproduce the bug.

Can you edit this JSFiddle and try to reproduce the bug?. Thanks.

https://jsfiddle.net/opbfw71j/

jonlepage commented 5 years ago

jsfiddle is not the same debug engine than vscode. Please take a look on the debugger options. You can see the break points.

You should never use try and catch for conditional. Use only if you need to call a crash and get information on the crash.

In this context you use caught exception for conditional value, so this tell to vscode to make my app break and alert by crash. So i fixed the issue by replace by a classic conditional. Is also more fast and performance friendly.

seiyria commented 5 years ago

We need a jsfiddle for issues like this because you're passing in data that the library doesn't expect, and we'd need to know why. This is a very valid place to use try/catch because JSON.parse would crash the initialization otherwise, and here we can recover from it without breaking normal usage. In fact, I'd say this is a great usage because it's letting you know right away that you're passing in bad data, and you're finding out about it.

In a case like this, performance is negligible - especially the difference between an if() statement and a try/catch.

jespirit commented 5 years ago
function getDataAttrib(element, optName) {
    var dataName = "data-slider-" + optName.replace(/_/g, '-');
    var dataValString = element.getAttribute(dataName);  // always a string

// if (typeof dataValString === 'string') {  // always "true"

    try {
        return JSON.parse(dataValString);
    }
    catch(err) {
        return dataValString;
    }
}

The type of dataValString is a string, so it would fail to parse arrays like "[0, 5, 10]" and instead of parsing strings as numbers, you get a string back: "400".

JSON.parse() is used to correctly parse JSON. For example, the following example would not display the ticks because it has invalid JSON.

<input id="slider1" data-slider-handle="square" data-slider-ticks="[0x, 5x, 10x]">

Correct:

<input id="slider1" data-slider-handle="square" data-slider-ticks="[0, 5, 10]">
jonlepage commented 5 years ago

ok, i understand. This is the context where i use your slider. https://github.com/djmisterjon/PIXI.ProStageEditor/blob/dfb82f637f95395cfd836efc7356c0a3c2b9c4e4/js/plugins/pixiMapEditor.js#L964

and the html code https://github.com/djmisterjon/PIXI.ProStageEditor/blob/dfb82f637f95395cfd836efc7356c0a3c2b9c4e4/js/iziToast/pixiMapEditor_HTML.js#L286

i don't know why and where i did a mistake, but i remove the try and all work fine.:) Sorry for the inconvenience. I guess I have a special context.

seiyria commented 5 years ago

We really, really need a jsfiddle. This helps us isolate if it's our code or your environment. Showing us code without a standalone executable environment does not help either of us in this regard.

jonlepage commented 5 years ago

I understand, I think it comes from my environment. I maybe do a mistake somewhere and i use it in the wrong way, but the important thing is that it works. I use your slider for only one context, I found it very pretty and simple for use my game editor. So i don't think it can affect badly my project. sliderdn

So you can close this issue if you want without fear that this problem occurs with other users, or maybe wait if another will get same issue. @+ and tank guys for your time.