AppFlowy icon indicating copy to clipboard operation
AppFlowy copied to clipboard
Published 4 months ago • verified publisher icon AppFlowy-IO

fix: Twelve Hour parsing issue

Open AnsahMohammad opened this issue 2 years ago • 5 comments

This PR closes the Issue #4720

  • [X] Fix issue 4720
  • [X] Fix 12hour format bug
  • [X] Auto capitalize AM/PM in the formatter
  • [x] Set placeholder according to user time format
  • [ ] Add tests

Feature Preview

PR Checklist

  • [x] My code adheres to AppFlowy's Conventions
  • [x] I've listed at least one issue that this PR fixes in the description above.
  • [ ] I've added a test(s) to validate changes in this PR, or this PR only contains semantic changes.
  • [x] All existing tests are passing.

AnsahMohammad avatar Feb 23 '24 16:02 AnsahMohammad

Hi @AnsahMohammad, please add some tests to cover this functionality to avoid running into the same issue as the last PR.

richardshiue avatar Feb 24 '24 08:02 richardshiue

Hey @AnsahMohammad Do you need help adding a test for this?

Xazin avatar Feb 28 '24 12:02 Xazin

Hey @AnsahMohammad Do you need help adding a test for this?

Yes, thank you! I've been trying to find the test files related to mention_date_block, but I couldn't find them (I later got busy with university exams). Could you help me find the existing test files so that I can extend them?

AnsahMohammad avatar Feb 28 '24 12:02 AnsahMohammad

@Xazin I also found few additional bugs/ improvements that we can make:

Screencast from 28-02-24 06:11:14 PM IST.webm

  1. The parser doesn't seem to work properly when input is given as 12:30 AM or 12:30 PM. I suspect this might be due to the time format being "HH:mm a" instead of "hh:mm a" for the 12-hour format.

image

  1. Currently, the parser only works correctly if AM/PM is typed strictly in uppercase. Should we modify it to accept AM/PM in either uppercase or lowercase?
  2. Despite my time format being set to the 12-hour format, the placeholder displays the time in the 24-hour format when I click on the time field. Should this be changed?

Screencast from 28-02-24 06:17:41 PM IST.webm

AnsahMohammad avatar Feb 28 '24 12:02 AnsahMohammad

  1. The parser doesn't seem to work properly when input is given as 12:30 AM or 12:30 PM. I suspect this might be due to the time format being "HH:mm a" instead of "hh:mm a" for the 12-hour format.

I would have to look into it to be able to find out the reason why, but you can try to change the format and see if it helps.

  1. Currently, the parser only works correctly if AM/PM is typed strictly in uppercase. Should we modify it to accept AM/PM in either uppercase or lowercase?

If we add transforming the string to uppercase to the text formatter, wouldn't that solve this particular issue?

  1. Despite my time format being set to the 12-hour format, the placeholder displays the time in the 24-hour format when I click on the time field. Should this be changed?

Yeah that can be changed, seems like it's something I missed when adding support for User date/time format.

Xazin avatar Feb 28 '24 13:02 Xazin