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

Bundling in a widget, scope #126

Open
arturi opened this issue Feb 27, 2018 · 13 comments
Open

Bundling in a widget, scope #126

arturi opened this issue Feb 27, 2018 · 13 comments

Comments

@arturi
Copy link

arturi commented Feb 27, 2018

Hi, and thanks for the awesome polyfill! I’m thinking of using it in Uppy file uploading widget http://github.com/transloadit/uppy I’m working on, and was wondering if you think an option to add scope would be a good idea?

  1. Option to specify a container element, so that .focus-visible will only be added to elements inside that container; Then .js-focus-visible could also be added to that container.
  2. Option to set a custom class name for .focus-visible (and .js-focus-visible maybe), so that I could set it to something like uppy-u-focus-visible to match my CSS guidelines and not risk being overriden by page styles (widgets are included in the page which already has third party CSS).

I can possibly send a PR for this, but wanted to hear your thoughts first. Thanks!

@alice
Copy link
Member

alice commented Feb 27, 2018

This seems like a good idea. Could we have a kind of "registry" for container name -> custom class name, so that multiple widgets could use it? How would you specify these options to the library?

@alice
Copy link
Member

alice commented Feb 27, 2018

(Also your widget looks great, and has a great logo!)

@robdodson
Copy link
Collaborator

Also mentioned in #46

I actually have mixed feelings on this idea. I might lean a bit more toward not encouraging libraries to bundle their own version of the polfyill, but to instead inform the page author that the polyfill is required to use the library. That avoids the situation where multiple components are trying to scope or change the name for the class that the polyfill applies.

@alice
Copy link
Member

alice commented Feb 27, 2018

Hm, I think as long as components can avoid stomping on each other and the main page (hence the "registry" idea), it's not too bad.

Obviously for the real pseudo-selector this won't be possible, but I can understand wanting to keep things scoped while using the library.

@robdodson
Copy link
Collaborator

yeah that's reasonable

@arturi
Copy link
Author

arturi commented Mar 1, 2018

(Also your widget looks great, and has a great logo!)

Thanks! :)

I might lean a bit more toward not encouraging libraries to bundle their own version of the polfyill, but to instead inform the page author that the polyfill is required to use the library. That avoids the situation where multiple components are trying to scope or change the name for the class that the polyfill applies.

but I can understand wanting to keep things scoped while using the library.

Yeah, I understand this is intended as a polyfill, but the way it currently works is it’s adding classes to elements, not pseudo-classes, and when this functionality is actually implemented in browsers it will be different, so can’t just remove the polyfill like with Promises or Fetch.

From my standpoint I can either bundle it with Uppy as external dependency (if I can add scope, because I think can’t be tempering with the whole document that I have no knowledge about, right? Feels a bit invasive) or copy-paste the code and modify it slightly to include scope features or hardcode classnames and container. Feels weird to be asking users to include focus-visible before using Uppy, most will just ignore it, I think, and its convenient when you can just include one script and style link and be done with it.

Could we have a kind of "registry" for container name -> custom class name, so that multiple widgets could use it?

By that do you mean that we document which lib is using this and what class names its using? 🤔Like:

# uppy

`.uppy-focus-visible` and `.uppy-u-focus-visible`

# codemirror

`.cm-focus-visible` and `.cm-element-focus-visible`

How would you specify these options to the library?

I’m thinking we’ll need a way to use it programmatically, like:

const focusVisible = require('focus-visible')
focusVisible.init({
  containterClassName: '.uppy-focus-visible',
  elClassName: '.uppy-u-focus-visible'
})

@alice
Copy link
Member

alice commented Mar 1, 2018

I was thinking more like

focusVisible.init({global: false});  // default to "global" mode which "decorates" the whole document
focusVisible.decorate(myEl, 'uppy-focus-visible-container', 'uppy-u-focus-visible');

then in the library

function addFocusVisibleClass(el) {
  if (!globalMode && !isDescendantOfDecoratedElement(el))
    return;

  // rest of method
}

You could have more than one element "decorated" in the document, but they don't need to know about each other.

I'm not sure how to have the current "auto init in global mode" functionality coexist with this, though.

@sampotts
Copy link

Has there been much progress on this? I'm in the same boat, wanting to add it to https://github.com/sampotts/plyr to help with this issue: sampotts/plyr#905

@robdodson
Copy link
Collaborator

robdodson commented Jun 29, 2018 via email

@sampotts
Copy link

OK sure I'll take a look and try and get a PR together 👍

@Justineo
Copy link
Contributor

Or can we ship a separate ponyfill version?

Usage in Uppy can be:

import focusVisible from 'focus-visible-ponyfill'

focusVisible({
  scope: '.uppy', // a selector, defaults to `:root`
  className: 'uppy-focus-visible' // defaults to `focus-visible`
})

And the polyfill version can be just:

import focusVisible from 'focus-visible-ponyfill'

focusVisible()

@robdodson
Copy link
Collaborator

Hey folks, unfortunately I'm heads down on a project that launches in early november. Not sure if I'll be able to get to this before then :\

Not sure if @alice has time to take a look?

@anilanar
Copy link

We have a ponyfill in this PR and in this NPM package, which is slightly refactored. It's also supposed to work in scopes like iframes and shadow dom. Our primary problem with this project (and any that assumes a global window/document) is that it cannot be injected into iframes/shadow dom contexts programmatically.

Feel free to use if you need it.

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

6 participants