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

Allow replacing stylesheet to preserve the cascade #1

Merged
merged 4 commits into from
Dec 1, 2015
Merged

Allow replacing stylesheet to preserve the cascade #1

merged 4 commits into from
Dec 1, 2015

Conversation

henrahmagix
Copy link

Usually this library will append media queries in separate <style> elements.
However, doing this changes the cascade such that a media rule will be used over
a non-media rule. This is because the media rule is removed from its position
and appended after everything else. It is a common case that a non-media rule
will override a media rule, so this inversion of the cascade makes it very
difficult to style for all browsers using this polyfill.

Turn on this option by setting window.RESPOND_REPLACE_STYLES = true before
sourcing this script. It will remove every stylesheet it finds and move all CSS
into the media="all" stylesheet that is appended, minus the inactive media
query rules and those of other media types, like print. The other types are
appended in the usual manner.

@incuna/js Please review, ta! The files to review are README.md and src/respond.js; the two *.src.js files are autocompiled from src/respond.js.

This fixes scottjehl#325

@wytrych
Copy link

wytrych commented Nov 27, 2015

So what is this, a fork you want to push back?

@henrahmagix
Copy link
Author

Yes.

This branch has conflicts that must be resolved

Darn, I forgot that I branched off 1.4.2, not master. I found that master wasn't working. Now that I'm familiar with the code, I'll see if I can rebase this onto master and get it working.

@henrahmagix
Copy link
Author

This pull-request is first for internal review, then when in master we can use it in projects and make a pull-request to the source repo.

@henrahmagix henrahmagix force-pushed the replace-instead-of-append-styles branch from 5e3b8e0 to 19b021c Compare November 27, 2015 16:25
@henrahmagix
Allow replacing stylesheet to preserve the cascade
6339f5c 
Usually this library will append media queries in separate <style> elements.
However, doing this changes the cascade such that a media rule will be used over
a non-media rule. This is because the media rule is removed from its position
and appended after everything else. It is a common case that a non-media rule
will override a media rule, so this inversion of the cascade makes it very
difficult to style for all browsers using this polyfill.

Turn on this option by setting `window.RESPOND_REPLACE_STYLES = true` before
sourcing this script. It will remove every stylesheet it finds and move all CSS
into the `media="all"` stylesheet that is appended, minus the inactive media
query rules and those of other media types, like print. The other types are
appended in the usual manner.
@henrahmagix henrahmagix force-pushed the replace-instead-of-append-styles branch from 19b021c to 6339f5c Compare November 27, 2015 16:28
@henrahmagix
Copy link
Author

@wytrych Fixed, ready for review

@henrahmagix henrahmagix mentioned this pull request Nov 27, 2015
Open
@henrahmagix
Fix other media types using "all" media type.
5eda5f7 
Bug was introduced when DRYing code to the `insertCss` function.
@henrahmagix
Fix replaceStartIndex === -1 replace bug.
807eda5 
As a result of rebasing the `RESPOND_REPLACE_STYLES` option code onto
1c8ce14 (after previously being on 1.4.2), the
replace index was being calculated as -1, meaning the query could not be found
in the total styles. This was because comments are now being stripped.

To fix that, the stripping of comments and keyframes from styles are done
immediately in the `translate` function, and then stored with their replaced
urls if `RESPOND_REPLACE_STYLES === true`.
@henrahmagix
Copy link
Author

@wytrych I introduced two bugs in the rebase. They're fixed now.

I was sure I had tested it, but I must have not been using the rebased code when testing in a project.

@wytrych
Copy link

wytrych commented Dec 1, 2015

There's an awful lot of code here in a library I have no knowledge of :( I'm not sure I can spot anything useful, getting to know the code would take me a lot of time.

@henrahmagix
Copy link
Author

I can explain it for you in person if that helps?

@wytrych
Copy link

wytrych commented Dec 1, 2015

That would basically mean running through the code, which still takes a lot of time

@henrahmagix
Copy link
Author

It'll be 5 minutes of explanation of the gist and a bit of detail regarding the changes here, then another 5-10 of answering questions you have.

@wytrych
Copy link

wytrych commented Dec 1, 2015

ok after lunch?

@henrahmagix
Copy link
Author

👍

wytrych pushed a commit that referenced this pull request Dec 1, 2015
Merge pull request #1 from incuna/replace-instead-of-append-styles
1288276 
Allow replacing stylesheet to preserve the cascade
@wytrych wytrych merged commit 1288276 into master Dec 1, 2015
@wytrych wytrych deleted the replace-instead-of-append-styles branch December 1, 2015 15:41
@henrahmagix
Copy link
Author

Thanks!

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.

Final cascaded rule does not work, always override by last Media Query
2 participants
@henrahmagix @wytrych