-
Notifications
You must be signed in to change notification settings - Fork 329
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
ENH - Update bundling and vendoring (JS) #1955
Conversation
webpack.config.js
Outdated
// Extracts CSS for each JS file that includes CSS | ||
{ loader: MiniCssExtractPlugin.loader }, | ||
{ | ||
// Interprets `@import` and `url()` like `import/require()` and will resolve them |
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.
Needed so that we can include FA directly in our stylesheets
webpack.config.js
Outdated
// Font vendoring and management - will separate FA and export the font files | ||
test: /\.(woff|woff2|eot|ttf|otf)$/i, | ||
type: 'asset/resource', | ||
generator: { | ||
filename: 'vendor/fontawesome/webfonts/[name][ext]' | ||
} | ||
},], |
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.
This replaces the previous approach where we copied the files from node_modules
and instead resolves the import
and extracts the font files to our vendor
directory
Looks good to me, restarted the test as the failure was unrelated, and likely due to a homebrew update. |
Thanks for the review @Carreau, since there do not seem to be any blockers, I will go ahead and merge 🚀 |
This PR is an attempt to improve our approach to bundling theme assets.
As you go through the PR, you will notice some changes, such as:
@import "variables/bootstrap";
imported twice? #1788)Closes: #1613
Closes: #1788