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

[WIP] Flatpickr Improvements #119

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

justinbeaty
Copy link
Contributor

@justinbeaty justinbeaty commented Mar 4, 2025

First commit (e9f2dc0) is a backup of the commit dropped from #106.

Second commit (db1d96b) was me working on improving our date picker:

Use flatpickr time selector

Product custom options has date, datetime, and time options. Below is how they appear on main:
image

Below is how the latter two now appear with this PR:
image

Luckily, there is no concept of timezones in the Magento product option, it's just a time (i.e. "09:00") that gets converted to 24h format for the DB. But actually I didn't finish the saving time values to the DB, that needs to be done in this file, currently they used some regex but maybe there's a better way to parse it.

Fix <depends> config node

In System > Config > Catalog > Date & Time Custom Options, all the config nodes depended on the JS calendar being enabled, but this doesn't make sense as all the options are used when the JS is disabled.

Mage_Core_Block_Html_Date updates

I know you didn't touch this class much, but we will need to break some compatibility here to enable time only inputs. Mage has an enableTime option, but it doesn't have a way to enable / disable the date picker separately from the time input.

So far I've added a new setConfig() method intended to control flatpickr options not present on the current PHP class. I think we can expand this class to support flatpickr directly instead of hacking around it in JS. For example, can we convert to flatpickr's format in PHP instead of JS? Right now the format is under config.ifFormat, so maybe add support for config.fpFormat. Maybe detect which one is present in JS act support both?

JS updates

Flatpickr wasn't working great on mobile for me, and it was caused by the instance not loading until the input has a focus or click event, and which time flatpickr would load, detect mobile, and turn it into a native input. I changed this so that inputs are initialized during the Calendar.setup() call.

I also had to fix one character in strftimeToDateConvertionMap related to AM / PM. That was '%p': 'A' -> '%p': 'K'. It seems flatpickr uses it's own slightly custom set of formatting tokens.

Note: there's a temp debug line in the JS to set the format to m/j/Y h:i K. But it seems that format is valid for all of date, datetime, and time pickers...

@justinbeaty
Copy link
Contributor Author

@fballiano are you interested and/or have time to work on this at all? I figure you have some ideas for this.

@justinbeaty
Copy link
Contributor Author

@fballiano -- just tagging you again in case you didn't see this.

@fballiano
Copy link
Contributor

Sorry, I thought I could finish the captcha more quickly and then answer here but...
I'll still check this out after the captcha cause I want to have it finished sooner than later, then it's probably time for 25.3 release and then I'd work on this one if that's ok with you.
But I'll also have to travel to phpcon netherlands later this month so it's gonna be a busy march 😅

@justinbeaty
Copy link
Contributor Author

No problem, just making sure you saw it. Worst case, we just add the one line to support the dialog usage before release:

const flatpickrOptions = {
    // ...
    static: !!inputEl.closest('dialog'),
}

@fballiano
Copy link
Contributor

can we convert to flatpickr's format in PHP instead of JS? Right now the format is under config.ifFormat, so maybe add support for config.fpFormat. Maybe detect which one is present in JS act support both?

mmm I lean toward thinking that I'd prefer to keep the php as agnostic as possibile, in the sense that these js libraries come and go and the less we're tied to them the more we're free. and to have it both in php and js IMHO could bring more possible bugs if we change one and forget the other.

Flatpickr wasn't working great on mobile for me, and it was caused by the instance not loading until the input has a focus or click event, and which time flatpickr would load, detect mobile, and turn it into a native input. I changed this so that inputs are initialized during the Calendar.setup() call.

I can't test the backend "for real" cause it seems I can't login via passkey on mobile, very weird.

But It seems to me you already did everything in this PR right?

@justinbeaty
Copy link
Contributor Author

mmm I lean toward thinking that I'd prefer to keep the php as agnostic as possibile [...] and to have it both in php and js IMHO could bring more possible bugs if we change one and forget the other.

Yes, I had the same concern about supporting formats in both JS and PHP, was sort of just throwing out ideas. So then let's just continue to use ifFormat which I understand is strftime. But, we need to review the mapping in the JS file because there was at least one wrong entry, and we might not even need all the tokens if there's no corresponding flatpickr token.

But It seems to me you already did everything in this PR right?

The mobile thing should be fixed, but I don't have an up to date Android phone to test. The problem was a combination of the lazy loading and appearance: none. I found it when on mobile I'd click a date input and it would disappear.

The main TODO here is the combination datetime, and regular time inputs. Flatpickr seems nice that even with native mobile inputs, it formats the date how we ask it to. Before time inputs sent data as multiple fields (hour, minute, am/pm), but now it would all be crammed into $_POST['date'] in the format like 3/27/2025 8:23 AM. We need to parse that to extract day, month, year, hour (24), minute. No timezones are involved, we just store those values in the DB.

I can't test the backend "for real" cause it seems I can't login via passkey on mobile, very weird.

Hm, I also cannot login via mobile (Chrome on iOS). I never disabled password auth so I didn't notice.

2025-03-07T16:16:42+00:00 ERR (3): 
lbuchs\WebAuthn\WebAuthnException: Invalid authenticatorData input in /home/www/maho/vendor/lbuchs/webauthn/src/Attestation/AuthenticatorData.php:56
Stack trace:
#0 /home/www/maho/vendor/lbuchs/webauthn/src/WebAuthn.php(438): lbuchs\WebAuthn\Attestation\AuthenticatorData->__construct()
#1 /home/www/maho/app/code/core/Mage/Admin/Model/User.php(433): lbuchs\WebAuthn\WebAuthn->processGet()
#2 /home/www/maho/app/code/core/Mage/Admin/Model/Session.php(139): Mage_Admin_Model_User->authenticate()
#3 /home/www/maho/app/code/core/Mage/Adminhtml/controllers/IndexController.php(68): Mage_Admin_Model_Session->prelogin()
#4 /home/www/maho/app/code/core/Mage/Core/Controller/Varien/Action.php(413): Mage_Adminhtml_IndexController->preloginAction()
#5 /home/www/maho/app/code/core/Mage/Core/Controller/Varien/Router/Standard.php(251): Mage_Core_Controller_Varien_Action->dispatch()
#6 /home/www/maho/app/code/core/Mage/Core/Controller/Varien/Front.php(176): Mage_Core_Controller_Varien_Router_Standard->match()
#7 /home/www/maho/app/code/core/Mage/Core/Model/App.php(353): Mage_Core_Controller_Varien_Front->dispatch()
#8 /home/www/maho/app/Mage.php(628): Mage_Core_Model_App->run()
#9 /home/www/maho/public/index.php(51): Mage::run()
#10 {main}```

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants