-
-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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
Angular: Include object configured styles #24768
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!!
@@ -66,6 +66,9 @@ exports.getWebpackConfig = async (baseConfig, { builderOptions, builderContext } | |||
|
|||
// Options provided by user | |||
...builderOptions, | |||
styles: builderOptions.styles | |||
?.map((style) => (typeof style === 'string' ? style : style.input)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@valentinpalkovic hi. I think there is a slight mistake here since first you use map
to take the string, but the filter
will always have a string. Now objects with "inject": false
will not be included in the build as separate files, now they will always be in the main.css
file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for pointing this out. You're right! Fix incoming: #27108
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@valentinpalkovic Thanks for the answer. It’s also worth noting that even if you swap the filter
and map
, files with “inject”: false
will not be included in the final build as separate files. 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe I misunderstood the inject
property in the past. Are you saying we shouldn't filter at all, but inject
only means whether the CSS is bundled in or outputted as a separate file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok so inject: false
just excludes the bundle from the injection to index.html
file, but the file will remain in the folder. inject: true
includes the file to index.html
. Example:
As I understand it, the problem was that files with inject: true
were not included in theindex.html
file or were not included in the main.css
file (not knowing exactly how the storybook builder works). After this fix, they will now be in the main.css
file, but files with inject: false
will not be created at all in the build (dist
) folder.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But that's great. Then I can kind of revert the work of this PR and do an internal mapping of styles defined as strings to the object notation. For the user, nothing changes, but things seem to work then!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it sounds like a great idea 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've put together a fix here: #27108
@andriy-statsenko-lightit Would you mind to test the following canary release and give me some feedback? 0.0.0-pr-27108-sha-2470133a
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@valentinpalkovic yeah, it works fine with different styles
arrays: string only, string + objects, and objects only. Thanks!
Closes #24683
Closes #17711
What I did
Support styles, which are configured as an object
Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
@storybook/angular
and replacenode_modules/@storybook/angular/dist
with build outputDocumentation
MIGRATION.MD
Checklist for Maintainers
When this PR is ready for testing, make sure to add
ci:normal
,ci:merged
orci:daily
GH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found incode/lib/cli/src/sandbox-templates.ts
Make sure this PR contains one of the labels below:
Available labels
bug
: Internal changes that fixes incorrect behavior.maintenance
: User-facing maintenance tasks.dependencies
: Upgrading (sometimes downgrading) dependencies.build
: Internal-facing build tooling & test updates. Will not show up in release changelog.cleanup
: Minor cleanup style change. Will not show up in release changelog.documentation
: Documentation only changes. Will not show up in release changelog.feature request
: Introducing a new feature.BREAKING CHANGE
: Changes that break compatibility in some way with current major version.other
: Changes that don't fit in the above categories.🦋 Canary release
This PR does not have a canary release associated. You can request a canary release of this pull request by mentioning the
@storybookjs/core
team here.core team members can create a canary release here or locally with
gh workflow run --repo storybookjs/storybook canary-release-pr.yml --field pr=<PR_NUMBER>