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

feat(rollup-plugin): add stylesheetConfig to rollup-plugin options #921

Merged
merged 1 commit into from
Jan 8, 2019
Merged

feat(rollup-plugin): add stylesheetConfig to rollup-plugin options #921

merged 1 commit into from
Jan 8, 2019

Conversation

nrobertdehault
Copy link
Contributor

Details

Add stylesheetConfig option to @lwc/rollup-plugin.

Fixes #920.

Does this PR introduce a breaking change?

  • Yes
  • No

@salesforce-cla
Copy link

salesforce-cla bot commented Jan 7, 2019

Thanks for the contribution! It looks like @nrobertdehault is an internal user so signing the CLA is not required. However, we need to confirm this.

pmdartus
pmdartus requested changes Jan 7, 2019
View reviewed changes
packages/@lwc/rollup-plugin/src/__tests__/fixtures/expected_compat_config_simple_app.js
return (
"\n" +
(nativeShadow
? ":host {color: var(--lwc-my-color);}"
Copy link
Member

Choose a reason for hiding this comment

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

This snippet has not been updated? The CSS variable has not been resolved from myCssResolver.

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, done.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Any idea why we don't use Jest snapshots here?

Copy link
Member

Choose a reason for hiding this comment

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

Jest snapshot works great for HTML and javascript object. However, I find it cumbersome to use with javascript code since it's hard to spot a syntax error.

Copy link
Contributor Author

@nrobertdehault nrobertdehault Jan 7, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

@pmdartus This snippet use the default resolver i.e. current behaviour, see expected_default_config_simple_app_css_resolver.js below for the one using a custom resolver.

@salesforce-best-lwc-internal
Copy link

Benchmark results

Base commit: 7c94042 | Target commit: 38ad996

@amateurhuman
Copy link

@nrobertdehault you've been added to the Salesforce org, once you've accepted the invitation you can refresh this pull request to clear the CLA check

diervo
diervo approved these changes Jan 7, 2019
View reviewed changes
Copy link
Contributor

@diervo diervo left a comment

Choose a reason for hiding this comment

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

If PM is happy with the test LGTM

@pmdartus pmdartus merged commit ca43c83 into salesforce:master Jan 8, 2019
@pmdartus
Copy link
Member

pmdartus commented Jan 8, 2019

Thanks, @nrobertdehault for the PR.

@nrobertdehault nrobertdehault deleted the stylesheetconfig-rollup-plugin branch January 8, 2019 12:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pmdartus pmdartus pmdartus approved these changes

@diervo diervo diervo approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@nrobertdehault @amateurhuman @pmdartus @diervo