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

add css option - fixes #24 #25

Closed
wants to merge 2 commits into from
Closed

add css option - fixes #24 #25

wants to merge 2 commits into from

Conversation

SamVerschueren
Copy link
Collaborator

Added support for a css option and it seems to work well. Feedback is more then welcome.

I am not sure if pageres should support a css file, but I think it would be a nice addition. Should screenshot-stream support a css file as well then? Or should pageres detect if it's a file, read the css file and pass the content of that file directly to screenshot-stream?

// @sindresorhus

Type: `string`

Apply CSS styles to the webpage.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added this block here, not sure if it should be added at the bottom of the options list? Not sure about the description either.

  1. Apply custom CSS styles to the webpage.
  2. Apply custom CSS styles.

Copy link
Contributor

Choose a reason for hiding this comment

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

Apply custom CSS to the webpage.

@kevva
Copy link
Owner

kevva commented Jan 29, 2016

Oh, great! I was working in this some time ago and I was using https://github.com/sindresorhus/file-url for files and used that in the href.


if (css) {
var style = document.createElement('style');
style.type = 'text/css';
Copy link
Contributor

Choose a reason for hiding this comment

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

Moot as it's the default.

@sindresorhus
Copy link
Contributor

Should screenshot-stream support a css file as well then?

Sure.

@SamVerschueren
Copy link
Collaborator Author

Added support for a css file but it's not working. Commented out the test. Anyone has an idea? Doesn't phantom support loading local files? Should I read the content of the file and just add it in the style tag?

@SamVerschueren
Copy link
Collaborator Author

Didn't seem to work with a local file in phantomjs. So now I read the file and inject the content in the page.

@@ -49,6 +49,10 @@ module.exports = function (url, size, opts) {
es5shim = fs.readFileSync(es5Shim, 'utf8');
}

if (/\.css$/.test(opts.css)) {
opts.css = fs.readFileSync(opts.css, 'utf8');
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Is this ok? Or should I use path.join(process.cwd(), opts.css)?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's ok. fs does that internally already.

@SamVerschueren
Copy link
Collaborator Author

Fixed description and merge conflict.

Will do a PR in pageres if this one lands to add a css method.

@SamVerschueren
Copy link
Collaborator Author

ping @kevva @sindresorhus :)

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

3 participants