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

DatePicker: disable input on mobile #4592

Open
wants to merge 4 commits into
base: rel-1.2
Choose a base branch
from

Conversation

stsrki
Copy link
Collaborator

@stsrki stsrki commented Feb 11, 2023

Closes #4577

This PR disables the input it DisableMobile is enabled. I have also set DisableMobile with default false, par flatpickr documentation.

The only problem is that the input is shown as grayed like it has a disabled state. I don't know if that is the right behavior for the flatpickr.

Example

image

@Mr-Pearce do you think this is OK?

@Mr-Pearce
Copy link

It will probably confuse users when the form is greyed out as if it is disabled but is enabled just that the keyboard wont show up.

@David-Moreira
Copy link
Contributor

I have to agree with Mr.Pearce, If I'm a user I would think its disabled by some reason.
Isn't there a better way to represent that text is disabled but you can interact with it?

@@ -52,7 +52,7 @@ export function initialize(element, elementId, options) {
clickOpens: !(options.readOnly || false),
disable: options.disabledDates || [],
inline: options.inline || false,
disableMobile: options.disableMobile || true,
disableMobile: options.disableMobile || false,
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe I'm dumb, what is this? options.disableMobile || false? is this some kind of way to coalesce?

Comment on lines 42 to 43
allowInput: !(options.disableMobile || false),
altInput: !(options.disableMobile || false),
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this some kind of way to coalesce is that it? I don't get the usage of the or logic here.

Comment on lines +42 to +43
allowInput: !(isMobile() && (options.disableMobile || false)),
altInput: !(isMobile() && (options.disableMobile || false)),
Copy link
Contributor

Choose a reason for hiding this comment

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

My mind can't think today :D Is this right? Does it handle every case?
Might be a better read if you just bring the isMobile() out.
I.E
!IsMobile() || (isMobile() && !options.disableMobile)

But I think it's those pipes that are killing me, again is it a coalesce operation?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The logic is actually broken on flatpickr side. They don't detect the mobile device properly. So I was trying to come up with a workaround, even pushed a PR on their repo, but it still seems not to detect all of the mobile devices.

@David-Moreira
Copy link
Contributor

@stsrki friendly reminder, this PR sitting here

@stsrki
Copy link
Collaborator Author

stsrki commented Aug 1, 2023

@stsrki friendly reminder, this PR sitting here

I know. I still didn't have much time to fix an error on flatpickr side. Which we must do before even applying it here.

@stsrki stsrki self-assigned this Nov 12, 2023
@David-Moreira
Copy link
Contributor

Just a friendly reminder, @stsrki

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.

3 participants