Skip to content

[Enhancement] Add parentElement parameter to use/ref method #126

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
Closed
wants to merge 2 commits into from
Closed

[Enhancement] Add parentElement parameter to use/ref method #126

wants to merge 2 commits into from

Conversation

Craga89
Copy link

@Craga89 Craga89 commented May 24, 2016 •
edited
Loading

Based on #65, rebased onto current master and cleaned up some-what. This allows the user to specify an optional parentElement to inject the link/style tag into at runtime, using the use/ref method:

var style = require("style/useable!css!./file.css");
var iframe = document.getElementsByTagName('iframe')[0];
style.use(iframe.contentDocument.head); // = style.ref(iframe.contentDocument.head);
style.unuse(); // = style.unref();

Useful for those of us rendering into iframes!

@ephys
Copy link

ephys commented Sep 26, 2016

Also deeply useful for inserting css into shadow roots, any chance of this being merged in the near future ?

worldsense-tms
worldsense-tms reviewed Sep 30, 2016
View reviewed changes
Copy link

@worldsense-tms worldsense-tms left a comment

Choose a reason for hiding this comment

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

A wild reviewer appears! Consider all my comments optional.

This change is really cool and would benefit my project tremendously.

addStyles.js
var styleElement = document.createElement("style");
styleElement.type = "text/css";
insertStyleElement(options, styleElement);
insertStyleElement(styleElement, parentElement, options);
Copy link

@worldsense-tms worldsense-tms Sep 30, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

I believe you need to call parentElement.ownerDocument.createElement() instead of document.createElement(). One alternative is to call adoptNode.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, probably better change this.

addStyles.js
var styleElement, update, remove;

if (options.singleton) {
var styleIndex = singletonCounter++;
styleElement = singletonElement || (singletonElement = createStyleElement(options));
styleElement = singletonElement || (singletonElement = createStyleElement(parentElement, options));
Copy link

Choose a reason for hiding this comment

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

When parentElement changes, the singletonElement should perhaps change as well?

@michael-ciniawsky
Copy link
Member

@Craga89 Could you please close and reopen the PR to trigger the CLA Bot agian && rebase against current master 🙃 ?

ekulabuhov
ekulabuhov suggested changes Mar 13, 2017
View reviewed changes
Copy link
Member

@ekulabuhov ekulabuhov left a comment

Choose a reason for hiding this comment

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

Needs to include some unit tests. Happy to help with that.

README.md
@@ -45,6 +45,16 @@ Styles are not added on `require`, but instead on call to `use`/`ref`. Styles ar

Note: Behavior is undefined when `unuse`/`unref` is called more often than `use`/`ref`. Don't do that.

#### Iframe support
Copy link
Member

Choose a reason for hiding this comment

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

Might need more general header here

ekulabuhov
ekulabuhov reviewed Mar 13, 2017
View reviewed changes
addStyles.js
var lastStyleElementInsertedAtTop = styleElementsInsertedAtTop[styleElementsInsertedAtTop.length - 1];
if (options.insertAt === "top") {
if(!lastStyleElementInsertedAtTop) {
head.insertBefore(styleElement, head.firstChild);
parentElement.insertBefore(styleElement, parentElement.firstChild);
Copy link
Member

Choose a reason for hiding this comment

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

There's an overlap with #135 here.

@ekulabuhov ekulabuhov mentioned this pull request Mar 13, 2017
Merged
joshwiens
joshwiens requested changes Mar 15, 2017
View reviewed changes
Copy link
Member

@joshwiens joshwiens left a comment •
edited
Loading

Choose a reason for hiding this comment

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

We are going to need to pick either #126 & #135.

1.) We have two features with overlapping functionality at a high level.
2.) #135 just needs tests and the author of this pull request hasn't responded
3.) #135 achieves the same thing using a method consistent with existing functionality

Before we start blowing up notification feeds for reviews / labeling and adding Issues to milestones we should be looking at open pull requests for duplication like this and chose what best solves the problem.

we should accept #135 and close #126

@michael-ciniawsky
Copy link
Member

Closes #44

@michael-ciniawsky michael-ciniawsky mentioned this pull request Mar 17, 2017
Closed
@ekulabuhov
Copy link
Member

@d3viant0ne good points. Let's focus our efforts on getting #135 merged.
Thank you everybody involved.

@ekulabuhov ekulabuhov closed this Mar 17, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ekulabuhov ekulabuhov ekulabuhov requested changes

@worldsense-tms worldsense-tms worldsense-tms left review comments

@joshwiens joshwiens joshwiens requested changes

@bebraw bebraw Awaiting requested review from bebraw

@michael-ciniawsky michael-ciniawsky Awaiting requested review from michael-ciniawsky

Assignees
No one assigned
Labels
pr_action: Review type: Feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@Craga89 @ephys @michael-ciniawsky @ekulabuhov @bebraw @joshwiens @worldsense-tms