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

Base support for web components #83

Closed
xdev1 opened this issue Oct 6, 2021 · 6 comments
Closed

Base support for web components #83

xdev1 opened this issue Oct 6, 2021 · 6 comments

Comments

@xdev1
Copy link
Contributor

xdev1 commented Oct 6, 2021

First, many thanks for this great project.
vanillajs-datepicker is remarkably flexible, but you currently cannot really use it with custom elements in shadow DOM.
There are two showstoppers:

  1. Option container should not only allow a string vaue but also an HTMLElement instance (as document.querySelector will not find elements in the shadow DOM).

    These lines here:

    const container = options.container ? document.querySelector(options.container) : null;
    if (container) {
    config.container = container;
    }

    could be replaced by:

    const container =
        options.container instanceof window.HTMLElement
          ? options.container
          : typeof options.container === 'string'
          ? document.querySelector(options.container)
          : null;
    
      if (container) {
        config.container = container;
      }
  2. document.activeElement is not working if a corresponding input element is in shadow DOM. See:

    if (element !== document.activeElement) {
    return;
    }

    Couldn't that just be replaced with the following?

    if (!datepicker.active) {
      return;
    }

Would be extremely great if these two issues could be fixed. Let me know if I shall prepare a PR. TYVMIA.

xdev1 added a commit to mcjazzyfunky/obsolete-vanillajs-datepicker-fork that referenced this issue Oct 6, 2021
@xdev1
Copy link
Contributor Author

xdev1 commented Oct 19, 2021

@mymth May I ask, whether you already know when the next version of vanillajs-datepicker will be released. Frankly, the above mentioned little issues are complete showstopper for the date selectors in my project currently (and everyone else who might use vanillajs-datepicker in custom elements). Forking just because of minor issues would not make sense at all and all other date pickers I've checked are not as flexible as this one, so using vanillajs-datepicker is currently my only option. At least a very soon beta release including the above mentioned changes (see PR #84) would be really, really helpful. TYVMIA.

@mymth
Copy link
Owner

mymth commented Nov 1, 2021

I don't have fixed date yet, but am aiming to get it done by the holidays. Releasing beta version is a good idea. I'll consider it.
Thank you for the PR and suggestion.

mymth added a commit that referenced this issue Nov 1, 2021
#83 - base support for web components
@xdev1
Copy link
Contributor Author

xdev1 commented Nov 1, 2021

@mymth Many thanks for the merge.
Regarding the "beta relase":
I've seen the other day, that yarn add github:mymth/vanillajs-datepicker will work with vanillajs-datepicker out of the box (many thanks to @brianzinn for that hint in some other issue), so I personally do not need a "beta release" any longer. Thanks.

@xdev1
Copy link
Contributor Author

xdev1 commented Nov 29, 2021

@mymth FYI: I've overlooked two other places which should be changed.
There are two further checks whether an input element is focused or not.
Instead of something like inputElement === document.activeElement (which is not working for input elements in shadow DOM) we should use inputElement.matches(':focus') instead.

Only the two places in inputFieldListeners.js and Datepicker.js are important (the unit tests can stay as is):
https://github.com/mymth/vanillajs-datepicker/search?q=document.activeElement

I'll prepare a further PR as soon as #91 is merged.

@mymth
Copy link
Owner

mymth commented Dec 4, 2021

Haven't pushed yet, but I already changed it as I found a bug from your change (It's nitpicking level, though—updating on blur didn't work when the picker is hidden by ESC key press) and its fix and the change were related.

I did it with adding a function that evaluates el.getRootNode().activeElement === el instead of matches(':focus'). I'm not sure which is actually better. If you have a clear answer, please let me know.

By the way, I’ll probably postpone merging #91 until after the next release in order to focus on making other changes planed earlier available as soon as possible. (I merged this issue because it's related to one of them).

So you don’t need to PR. Thank you tough for the offer.

mymth added a commit that referenced this issue Dec 29, 2021
… input is not updated on blur if picker is hidden manually
mymth added a commit that referenced this issue Dec 29, 2021
… input is not updated on blur if picker is hidden manually

* Merge branch 'dev'
@mymth
Copy link
Owner

mymth commented Jan 17, 2022

I'll close this as this was resolved by v1.2.0 release.

@mymth mymth closed this as completed Jan 17, 2022
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

No branches or pull requests

2 participants