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

Support <meteor-bundled-css /> pseudo-tag for controlling position of CSS bundle. (#24) #9657

Merged
merged 13 commits into from
Feb 21, 2018

Conversation

rcurrier666
Copy link
Contributor

This PR is for feature request #24 Order of merged-stylesheets.css in <head>. A new pseudo tag, <meteor-bundled-css>, has been created. When placed anywhere in the <head> of an app it will be replaced by the link to the bundled CSS in the output. If no tag is seen, then the bundle will be placed at the top of the <head> section as before so that this feature is backward compatible.

Two new tests have been created, one to verify that the tag is actually replaced by the bundle, and one to verify that the bundle is in the correct place. All old boilerplate tests are untouched (except for the addition of a flag to control whether the input HTML contains the tag) and continue to work (verifying that without the tag everything works as before).

[WIP question] Where should this be documented? I thought perhaps in the build section, though I don't see an obvious place to put it there. It also seems to be a non-obvious place for anyone to look. Or will it only appear in the release notes?

Otherwise, this PR should be good to go.

@rcurrier666
Copy link
Contributor Author

My unit tests succeeded but the one failure isn't any place that I touched:

S: tinytest - observeChanges - unordered - specific fields + selector on excluded fields : !!!!!!!!! FAIL !!!!!!!!!!!

@axelvaindal
Copy link
Contributor

In order to help tracking this PR in the corresponding feature request, here is the related link (as yours redirect to the 24th issue of the Meteor repository.

Meteor Feature Request #24.

@rcurrier666
Copy link
Contributor Author

With last night's commits, that should wrap up both the code and tests for both browser and Cordova for this feature. I just need someone to let me know where the documentation for this feature should go or if it isn't needed.

Copy link
Contributor

@hwillson hwillson left a comment

Choose a reason for hiding this comment

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

Thanks for submitting this PR @rcurrier666! I've added a few small formatting issue comments inline, but other than that I think we're just about all set here (I just need to finish some extra testing). Thanks again!

}) => [
}) => {
var headSections = head.split(/<meteor-bundled-css[^<>]*>/, 2);
// if (headSections.length > 1) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove this commented code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, thought I had removed this before.

dynamicHead,
'</head>',
'<body>',
].join('\n');
};
Copy link
Contributor

Choose a reason for hiding this comment

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

The indentation looks like a little off here - looks like lines 21 - 35 need to be indented.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The entire "return [" block needed to be indented. I also added a couple of blank lines to make it easier to see which lines go together.

@hwillson
Copy link
Contributor

Thanks @rcurrier666 - looks great! I've finished an additional round of testing, and things are working well. We have a PR review meeting today and this will be discussed.

@benjamn benjamn added this to the Release 1.6.2 milestone Feb 21, 2018
Copy link
Contributor

@benjamn benjamn left a comment

Choose a reason for hiding this comment

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

This is great! Let's just bump the package versions to 1.5.0 so that we can schedule this for Meteor 1.6.2, since it's a fairly significant change.

@abernix abernix changed the title [WIP] Feature request #24 - Meteor css bundle Meteor css bundle (#24) Feb 21, 2018
@abernix abernix changed the title Meteor css bundle (#24) Meteor css bundle Feb 21, 2018
@benjamn benjamn changed the title Meteor css bundle Support <meteor-bundled-css /> pseudo-tag for controlling position of CSS bundle. (#24) Feb 21, 2018
@benjamn benjamn merged commit 282b450 into meteor:devel Feb 21, 2018
skirunman added a commit to skirunman/guide that referenced this pull request Oct 14, 2018
Document use of `.meteorignore` meteor/docs#262 added in Meteor v1.5.2.1

Document use of `<meteor-bundled-css />` tag added in Meteor 1.7
meteor#720
meteor/meteor#9657

General clean up and re-write for clarification.
lorensr pushed a commit to meteor/guide that referenced this pull request Oct 14, 2018
Document use of `.meteorignore` meteor/docs#262 added in Meteor v1.5.2.1

Document use of `<meteor-bundled-css />` tag added in Meteor 1.7, fixes #720
#720
meteor/meteor#9657

General clean up and re-write for clarification.
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

Successfully merging this pull request may close these issues.

None yet

4 participants