Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Calendar] Fix broken dates when keyboard typed #995

Merged
merged 2 commits into from
Sep 6, 2019

Conversation

prudho
Copy link
Contributor

@prudho prudho commented Sep 4, 2019

Description

This PR aim to fix (or at least drastically reduce) the broken date format when it's typed through keyboard. It's a bit hacky, since it only reverse supposed days and month when the setting monthFirst is defined to true, but I think this little piece of code will cover 98% of cases.

Testcase

Before: JSFiddle
After: JSFiddle

Closes

#829
#986

@prudho prudho added lang/javascript Anything involving JavaScript state/awaiting-reviews Pull requests which are waiting for reviews labels Sep 4, 2019
@prudho prudho added this to the 2.8.0 milestone Sep 4, 2019
@y0hami y0hami added the type/bug Any issue which is a bug or PR which fixes a bug label Sep 4, 2019
@lubber-de
Copy link
Member

lubber-de commented Sep 4, 2019

🤔 Isn't the change assuming the provided text is always in "wrong" order when monthFirst = false? It would exchange those two values all the time.
I think we should rather replace the whole parser by a proper method which always uses a given dateFormat setting without the need to rely on heavy extra libraries like moment.js

I think adopting something like the following tiny but effective method is suitable
http://blog.stevenlevithan.com/archives/date-time-format
->
https://github.com/felixge/node-dateformat/blob/master/lib/dateformat.js

@prudho
Copy link
Contributor Author

prudho commented Sep 4, 2019

For users who use the "dd/mm/yyyy" format, they'll type as it, so the date will be always be corrected for them. People that use monthFirst: false are, for the vast majority, the same people that use the "dd/mm/yyyy" format (let's say 98% 😃).

I know that it's not the best solution, but write a brand new date parser, which will handle all dates specificities (timezones, format...), will need a heavy reflection (and we must not forget that calendar can also handle minutes, month and years only).

Copy link
Member

@lubber-de lubber-de left a comment

Choose a reason for hiding this comment

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

Lets also support a dot as divider, otherwise it is still failing
calendar_dot

src/definitions/modules/calendar.js Outdated Show resolved Hide resolved
Copy link
Member

@lubber-de lubber-de left a comment

Choose a reason for hiding this comment

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

LGTM
I also changed the jsfiddle with a new one to prove the dot usage is also fixed

Copy link
Member

@y0hami y0hami left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@exoego exoego left a comment

Choose a reason for hiding this comment

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

LGTM

@y0hami y0hami removed the state/awaiting-reviews Pull requests which are waiting for reviews label Sep 6, 2019
@y0hami y0hami merged commit 98f999f into fomantic:develop Sep 6, 2019
@PhilippGrashoff
Copy link

I also changed the jsfiddle with a new one to prove the dot usage is also fixed

Hi, I tested the jsFiddle https://jsfiddle.net/z6ghfcv9/, but its failing as in lubber_de's gif. It picks 12th of october if a dot is used as seperator

@lubber-de
Copy link
Member

@PhilippGrashoff
😱 I confirm, it still happened when using at least Edge/IE11 ...
I created another Hotfix #1004 which should work in any browser now
Please try this new jsfiddle https://jsfiddle.net/9caf67pv/

@prudho prudho deleted the fix-829 branch October 10, 2019 06:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lang/javascript Anything involving JavaScript type/bug Any issue which is a bug or PR which fixes a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants