Skip to content
This repository has been archived by the owner on Nov 29, 2023. It is now read-only.

Fix: Respect existing val of type='time' #980

Merged
merged 1 commit into from
Jun 28, 2023
Merged

Conversation

JGreenlee
Copy link
Contributor

Fixes enketo/enketo#33.

Regression and fix described there, enketo/enketo#33:

I found the change that caused this regression.
3accdd8#diff-caad802f244c59940e6441fa0684b96b8c791470596c23de562f204c2dd1da40

It look like when dropping support for IE, this:

// For IE11, we also need to strip the Left-to-Right marks \u200E...
const ds = `${new Date()
    .toLocaleDateString('en', {
        month: 'short',
        day: 'numeric',
        year: 'numeric',
    })
    .replace(/\u200E/g, '')} ${value.replace(
    /(\d\d:\d\d:\d\d)(\.\d{1,3})(\s?((\+|-)\d\d))(:)?(\d\d)?/,
    '$1 GMT$3$7'
)}`;
const d = new Date(ds);

was simplified to this:

const ds = `${new Date().toLocaleDateString('en', {
    month: 'short',
    day: 'numeric',
    year: 'numeric',
})}`;
const d = new Date(ds);

The intention with that commit was to remove .replace(/\u200E/g, ''), as IE support is no longer needed.
I think that ${value.replace(...) was removed by accident.


Here is my proposed fix, which makes it a bit clearer what is actually supposed to happen here.

const todayDate = new Date().toLocaleDateString('en', {
    month: 'short',
    day: 'numeric',
    year: 'numeric',
});
const valueTime = value.replace(
    /(\d\d:\d\d:\d\d)(\.\d{1,3})(\s?((\+|-)\d\d))(:)?(\d\d)?/,
    '$1 GMT$3$7'
);
const d = new Date(`${todayDate} ${valueTime}`);

I will put together a PR for this.

Copy link
Contributor

@eyelidlessness eyelidlessness left a comment

Choose a reason for hiding this comment

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

Thanks!

@lognaturel
Copy link
Contributor

The same raster test is failing as fixed in the last merged PR. So I think we can merge this and it should be green on master, right?

@lognaturel lognaturel merged commit 1a4b5ea into enketo:master Jun 28, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to prefill time fields (in version 7.x)
3 participants