Scribery / cockpit

Cockpit fork for work on Session Recording UI
http://www.cockpit-project.org/
GNU Lesser General Public License v2.1
6 stars 3 forks source link

Fix sign output in number padding #34

Closed spbnick closed 7 years ago

spbnick commented 7 years ago

@sabbaka, does this look right to you?

sabbaka commented 7 years ago

We may use something like Moment.js to handle everything related to time and date.

spbnick commented 7 years ago

Sure, but we aren't yet, and we shouldn't add dependencies too hastily. Cockpit probably has something already.

sabbaka commented 7 years ago

It's there already and also as a dependency for date time picker.

On Mon, Sep 18, 2017 at 7:41 PM, Nikolai Kondrashov < notifications@github.com> wrote:

Sure, but we aren't yet, and we shouldn't add dependencies too hastily. Cockpit probably has something already.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/Scribery/cockpit/pull/34#issuecomment-330298588, or mute the thread https://github.com/notifications/unsubscribe-auth/AFTQ5ZfRDD5RcYTlE4jpgEV4e1sHklpMks5sjqtBgaJpZM4PbPeN .

-- Kyrylo Gliebov Software Engineer Red Hat Czech

spbnick commented 7 years ago

Nice :) Feel free to fix this by switching over to it then :)

sabbaka commented 7 years ago

moment.js:737 Deprecation warning: moment construction falls back to js Date. This is discouraged and will be removed in upcoming major release. Please refer to https://github.com/moment/moment/issues/1407 for more info.

sabbaka commented 7 years ago

Will do this, also related to check in datetimepicker.

spbnick commented 7 years ago

Could you please convert this into an appropriate issue and close?

sabbaka commented 7 years ago

39 issue created.

sabbaka commented 7 years ago

Just one quick question - but why did you bother to change "a" to "i" - because there is no difference or this is an actual reason? :)

spbnick commented 7 years ago

Because i can be negative, a can't.

sabbaka commented 7 years ago

It's quite interesting, I have just tried both a and i and compared to output of moment.js and only two dates matched, others have difference in seconds.

sabbaka commented 7 years ago

screenshot from 2017-09-21 13-27-04

spbnick commented 7 years ago

What is the code producing this?

sabbaka commented 7 years ago
    let formatDateTime = function (ms) {
        let d = new Date(ms);
        let test1 = moment(d).format('YYYY-MM-DD HH:mm:SS');
        let test2 = (
            padInt(d.getFullYear(), 4) + '-' +
            padInt(d.getMonth() + 1, 2) + '-' +
            padInt(d.getDate(), 2) + ' ' +
            padInt(d.getHours(), 2) + ':' +
            padInt(d.getMinutes(), 2) + ':' +
            padInt(d.getSeconds(), 2)
        );
        if(test1 == test2) {
            console.log(test1 + test2 + 'true');
        }
        else {
            console.log(test1 + test2);
        }
    };
spbnick commented 7 years ago

Can you make a third string here, just pasting the parts together without padInt, and outputting them? Please also add spaces between output strings.

sabbaka commented 7 years ago
        let d = new Date(ms);
        let test1 = moment(ms).format('YYYY-MM-DD HH:mm:SS');
        let test2 = (
            padInt(d.getFullYear(), 4) + '-' +
            padInt(d.getMonth() + 1, 2) + '-' +
            padInt(d.getDate(), 2) + ' ' +
            padInt(d.getHours(), 2) + ':' +
            padInt(d.getMinutes(), 2) + ':' +
            padInt(d.getSeconds(), 2)
        );
        let test3 = (
            d.getFullYear() + '-' +
            d.getMonth() + 1 + '-' +
            d.getDate() + ' ' +
            d.getHours() + ':' +
            d.getMinutes() + ':' +
            d.getSeconds()
        );
        if(test1 == test2) {
            console.log(test1 + ' ' + test2 + ' ' + test3 + ' true');
        }
        else {
            console.log(test1 + ' ' + test2  + ' ' + test3 );
        }

screenshot from 2017-09-21 13-41-55

sabbaka commented 7 years ago

Sorry, my fault, I used wrong parameter for moment for seconds. Arghh

sabbaka commented 7 years ago

It was SS - for fractional seconds and for 00 seconds should be only ss.

spbnick commented 7 years ago

I see, it's OK :)

spbnick commented 7 years ago

@sabbaka, do you think we can merge this, since we decided against using a library?

spbnick commented 7 years ago

Thanks, merged!