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.

Sign up for GitHub

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

Jump to bottom

automatically inject css into head #138 #180

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

automatically inject css into head #138 #180

wants to merge 3 commits into from

Conversation

mreinstein
Copy link
Contributor

this removes the need for manually inserting the css into <head>

@akc42
Copy link

akc42 commented Sep 14, 2019

I think there could be a problem with this. I only just started tested using the polyfill today using Safari - all previous uses have been with Chrome, but...

I have <dialog> elements inside custom elements, and I have found that I don't want the css file imported into <head>. Instead it has to be imported into the shadowRoot of the custom element that uses it.

@mreinstein
Copy link
Contributor Author

mreinstein commented Sep 14, 2019 •
edited

@akc42 valuable use case, thanks for sharing! We could potentially modify the registerDialog function to accept some optional configuration around where to inject the style:

registerDialog(element, { cssParent: element.shadowRoot })

If the 2nd argument is missing or cssParent is invalid, keep defaulting to head.

open for suggestions!

@akc42
Copy link

akc42 commented Sep 15, 2019

I think you can actually work out automatically whether to inject into the shadowRoot or the document header. The branch of the polyfill I am actually using now is one cloned from the one made to support custom elements - see #168 and that finds out if there is a shadowRoot or whether to use the document.body

I am using contructable style sheets to do the insertion (see https://github.com/WICG/construct-stylesheets/blob/gh-pages/explainer.md) for what these are (see also npm package construct-style-sheets-polyfill). Note I am using lit-element which already has an ability to use this facility without the npm package.

For the efficiency reasons stated in this proposal, I guess this is also the approach you should take if you continue to do it automatically. However there is a downside as it appears that they are injected below the styles that are already there and so take precedence and this caused me problems - see below

So I also ended up not including some of the css (the piece just styling dialog) because I had already styled it differently in my different custom elements. In particular, in one particular case I was moving the dialog to sit just under where the user just clicked on the page (itself scaled with a css transform) and was using getBoundingClientRect() to workout the dialog size and do a calculation to add a style of top and left to the element. I had broken this code and the positioning was way off. I eventually found out that was because of margin:auto in the css, whereas I had explicitly tried to style it with margin:0 and that had not overridden.

(I think I took the simpler approach of removing the styling for the dialog css - maybe a better approach would have been to make an additional constructable style sheet for my styling and added that after the polyfill one so it overrode it)

@mreinstein mreinstein mentioned this pull request Oct 21, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@mreinstein @akc42