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

@bebraw bebraw Mar 13, 2017

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
if (options.singleton) {
var styleIndex = singletonCounter++;
styleElement = singletonElement || (singletonElement = createStyleElement(options));
styleElement = singletonElement || (singletonElement = createStyleElement(parentElement, options));
Copy link

@worldsense-tms worldsense-tms Sep 30, 2016

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

michael-ciniawsky commented Mar 13, 2017

@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
Contributor

@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

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

#### Iframe support
Copy link
Contributor

@ekulabuhov ekulabuhov Mar 13, 2017

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
if (options.insertAt === "top") {
if(!lastStyleElementInsertedAtTop) {
head.insertBefore(styleElement, head.firstChild);
parentElement.insertBefore(styleElement, parentElement.firstChild);
Copy link
Contributor

@ekulabuhov ekulabuhov Mar 13, 2017

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 suggested 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

michael-ciniawsky commented Mar 17, 2017

Closes #44

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

ekulabuhov commented Mar 17, 2017

@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

@bebraw bebraw Awaiting requested review from bebraw

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

3 more reviewers

@ekulabuhov ekulabuhov ekulabuhov requested changes

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

@joshwiens joshwiens joshwiens requested changes

Reviewers whose approvals may not affect merge requirements

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