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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add styleExtensions #4671

Merged
merged 3 commits into from Jan 2, 2019
Merged

Conversation

manniL
Copy link
Member

@manniL manniL commented Jan 2, 2019

In your css array, you have to write the corresponding extension when providing your stylesheets. While transitioning from SCSS to PostCSS in several projects, I thought: "Why not completely omitting the extension as we do it for JS resolves 馃?"

There we go!

Types of changes

  • New feature (a non-breaking change which adds functionality)

Checklist:

  • My change requires a change to the documentation.
  • I have updated the documentation accordingly. (PR: #)
  • I have added tests to cover my changes (if not applicable, please state why)
  • All new and existing tests are passing.

@@ -46,7 +46,7 @@ export default class Resolver {
return resolve(this.options.srcDir, path)
}

resolvePath(path, { alias, module } = {}) {
resolvePath(path, { alias, module, isStyle } = {}) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe we should move to isXYZ syntax generally for booleans here? 馃

Copy link
Member

Choose a reason for hiding this comment

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

I like this convention 馃憤

@manniL manniL requested review from pi0 and clarkdo January 2, 2019 18:52
@codecov-io
Copy link

codecov-io commented Jan 2, 2019

Codecov Report

Merging #4671 into dev will increase coverage by 0.04%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##              dev    #4671      +/-   ##
==========================================
+ Coverage   90.78%   90.82%   +0.04%     
==========================================
  Files          67       67              
  Lines        2236     2224      -12     
  Branches      545      544       -1     
==========================================
- Hits         2030     2020      -10     
+ Misses        187      185       -2     
  Partials       19       19
Impacted Files Coverage 螖
packages/config/src/config/_common.js 100% <酶> (酶) 猬嗭笍
packages/core/src/resolver.js 86.79% <100%> (+0.25%) 猬嗭笍
packages/config/src/options.js 94.11% <0%> (-2.1%) 猬囷笍
packages/vue-app/src/index.js 0% <0%> (酶) 猬嗭笍
packages/builder/src/builder.js 96.7% <0%> (+3.17%) 猬嗭笍

Continue to review full report at Codecov.

Legend - Click here to learn more
螖 = absolute <relative> (impact), 酶 = not affected, ? = missing data
Powered by Codecov. Last update 7dd33fe...2fb50c1. Read the comment docs.

galvez
galvez previously approved these changes Jan 2, 2019
Copy link

@galvez galvez left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Member

@pi0 pi0 left a comment

Choose a reason for hiding this comment

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

Nice enhancement. But I don't see any reason adding complexity for handling option as a function or string unless a specific use case exists. replace or append merge strategy would be enough and more consistent. Modules has the opprotounity to modify options in any manner they need :)

@@ -46,7 +46,7 @@ export default class Resolver {
return resolve(this.options.srcDir, path)
}

resolvePath(path, { alias, module } = {}) {
resolvePath(path, { alias, module, isStyle } = {}) {
Copy link
Member

Choose a reason for hiding this comment

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

I like this convention 馃憤

packages/config/src/options.js Outdated Show resolved Hide resolved
packages/config/src/options.js Outdated Show resolved Hide resolved
@@ -43,6 +43,21 @@ export function getNuxtConfig(_options) {
options.extensions = [options.extensions]
}

const styleExtensionDefaults = ['css', 'pcss', 'postcss', 'styl', 'stylus', 'scss', 'sass', 'less']
Copy link
Member

Choose a reason for hiding this comment

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

If defaults ordering would be important I suggest mentioning it in the docs too :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I'll create a doc PR afterwards anyway 鈽猴笍
Thanks for the heads up!

pi0
pi0 previously approved these changes Jan 2, 2019
@pi0 pi0 merged commit 471a32a into dev Jan 2, 2019
@pi0 pi0 deleted the feat-resolve-style-paths-to-omit-endings branch January 2, 2019 21:30
@manniL manniL mentioned this pull request Jan 3, 2019
2 tasks
@danielroe danielroe added the 2.x label Jan 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants