elado / next-with-less

Next.js + Less CSS Support
MIT License
143 stars 24 forks source link

Adds option to customize antd variables #2

Closed lgenzelis closed 3 years ago

lgenzelis commented 3 years ago

This PR allows antd users to provide a file to override whichever antd variable they want. I added instructions to use it to the README file.

Inspired by another plugin, next-plugin-antd-less

elado commented 3 years ago

Hi @lgenzelis, thanks for contributing! My intention was to keep this plugin as "dumb" as possible. I'm currently not using additionalData at all, but also haven't worked too much with Less.

So a couple of questions -

lgenzelis commented 3 years ago

You're welcome @elado ! ^^ I don't know how common is it to add overrides to additionalData, but I do know it is extremely common to override some of antd variables. It's the official way to do customize it.

Adding something like lessLoaderOptions would be ok, I guess. Less code here in the plugin, but more code for the users. It's your call :) We can still add an example of how we'd use lessLoaderOptions to customize antd in the README, since many users are probably going to want to do that.

lgenzelis commented 3 years ago

We could name that option additionalData or lessLoaderAdditionalData (I think lessLoaderOptions would be confusing, it's more similar to lessOptions than to additionalData).

Then, user usage for this case would be something like:

module.exports = withLess({
  future: {
    webpack5: true,
  },
  //....
  lessLoaderAdditionalData: content =>`${content}\n\n@import '${pathToLessFileWithVariables}';`,
}

Doesn't seem so terrible :)

elado commented 3 years ago

Looks like the official way suggests modifyVars (which is what my team uses).

I want to be more "future proof" so not restricting user to a single use case.

I'm advocating to something like:

module.exports = withLess({
  future: {
    webpack5: true,
  },

  lessLoaderOptions: { // passed as `options` to the loader, merged with `lessOptions:{javascriptEnabled: true}`
    lessOptions: {
      modifyVars: { .. },
    },
    additionalData: content => `${content}\n\n@import '${pathToLessFileWithVariables}';`,
    // any other less loader option...
  }
}

this will allow flexibility for additional less-loader options (existing and future)

dryleaf commented 3 years ago

I was wondering whether I could inject antd styles without using _app.js page. The example in this package relies merely on additional customization to NextJs's default _app.js.

jayliew commented 3 years ago

You're welcome @elado ! ^^ I don't know how common is it to add overrides to additionalData, but I do know it is extremely common to override some of antd variables. It's the official way to do customize it.

I just wanted to also chime in that I am also looking for a way to override some of the variables (in order to change color). Thank you all for working on this!

lgenzelis commented 3 years ago

I closed this PR and created a new one, #4, since we ended up going in a different direction that I originally described. You're absolutely right @elado , this is a better approach, it adds almost no complexity to the plugin, while still allowing users to customize as needed.

BTW, the reason why I prefer overriding the variables using additionalData instead of just modifyVars, is that with the latter you have to stop the server and do npm run dev again each time you modify a variable. Using additionalData to inject the variables file, hot reload works as usual. It makes tweaking the variables much easier :)

lgenzelis commented 3 years ago

I was wondering whether I could inject antd styles without using _app.js page. The example in this package relies merely on additional customization to NextJs's default _app.js.

@dryleaf I don't think that's possible. You need to import the styles somewhere, and _app is the place for that (since these are global styles).

dryleaf commented 3 years ago

@lgenzelis I created an issue#3, please have a look.

lgenzelis commented 3 years ago

@dryleaf yeah, I get that warning as well, but Idk if there's anyway around it. Just in case, note that this is hurting build time performance. Site execution is not affected.