Skip to content
This repository has been archived by the owner on Feb 12, 2022. It is now read-only.

Allow default unit to be added to spinbox if not present #1492

Merged

Conversation

interactivellama
Copy link
Contributor

I'd call this a bug, since I think spinbox was meant to work this way, but it is a new initialization option.

Fixes #1490.

@interactivellama interactivellama added this to the 3.11.2 milestone Aug 27, 2015
@swilliamset
Copy link
Contributor

White space trouble. Input starts with a space between the number and the unit but them removes the space.
sep 04 2015 15 50

Also if I set the default unit to something besides the accepted units including null I get weird behavior.

    $('#mySpinbox2').spinbox({
        value: '10 px',
        min: 0,
        max: 10,
        step: 0.1,
        // decimalMark: ',',
        units: ['px'],
        defaultUnit: 'ouch'
    });

sep 04 2015 15 54

    $('#mySpinbox2').spinbox({
        value: '10 px',
        min: 0,
        max: 10,
        step: 0.1,
        // decimalMark: ',',
        units: ['px'],
        defaultUnit: null
    });

sep 04 2015 15 55

Granted these are fringe cases but should have better handling.

Feel free to use the config blocks above to write additional unit tests.

@interactivellama
Copy link
Contributor Author

The defaults in our apps right now are no space whether tradition or intentional.
screen shot 2015-09-08 at 4 15 25 pm

since spinboxes do not traditionally use spaces within our applications
@interactivellama
Copy link
Contributor Author

Updated with non-allowed default unit test.

I am not adding null. Non-strings should not be used, nor accounted for.

For instance options.units better be an array, since it used .length without checking to see if it's an array.

@interactivellama interactivellama force-pushed the spinbox-default-units branch 2 times, most recently from 6e65aa7 to c949605 Compare September 8, 2015 22:13
@cmcculloh-kr
Copy link

Breaks for any 3+ digit units

$('#mySpinbox2').spinbox({
    value: '1,0pxs',
    min: 0,
    max: 10,
    step: 0.1,
    decimalMark: ',',
    units: ['pxs'],
    defaultUnit: 'pxs'
});

image

@interactivellama
Copy link
Contributor Author

Updated. Thanks.

// if set and default unit if not already present,
// and is an allowed unit, then add default unit
if (this.options.defaultUnit !== '' &&
this.options.defaultUnit !== value.slice(-2) &&

Choose a reason for hiding this comment

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

You'll probably want to adjust this to account for units longer than 2

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Refresh the page.

@cmcculloh-kr
Copy link

Please add a unit test for some unit/default unit combo that is 3+ digits and increments the spinbox at least once.

@interactivellama
Copy link
Contributor Author

@cmcculloh Added test that uses rem and increments. Great suggestions!

Also tests support for 3 character default unit
cmcculloh-kr pushed a commit that referenced this pull request Sep 10, 2015
Allow default unit to be added to spinbox if not present
@cmcculloh-kr cmcculloh-kr merged commit 14b7547 into ExactTarget:master Sep 10, 2015
@interactivellama interactivellama deleted the spinbox-default-units branch April 4, 2016 15:41
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants