Skip to content
This repository has been archived by the owner on Sep 29, 2023. It is now read-only.

Don't require the use of "less" #8

Closed
wants to merge 11 commits into from
Closed

Conversation

mrmurphy
Copy link

I was just cleaning up security warnings on our main App at work, and a bunch of them were for "less". I realized that the only reason we were using less at all was to require bs-ant-design.

I propose separating the Reason bindings from the importing of the css / less for the following reasons:

  • The bundled and minified CSS files for all versions of ant design are easily available on CDN.js. So it'd be more convenient in many situations for users of this package to use that CSS, rather than required use of a bundler and a less interpreter.
  • Currently, since this package directly imports the Antd css, if the consumer of this package wants to to customize the Ant design theme they have to fork the package (correct me if I'm wrong). If we leave the importing of the CSS to the consumer, they can customize the theme and add it to their page however they like without any changes to these bindings.

Thanks for the work!

@Enalmada
Copy link
Owner

Enalmada commented Sep 11, 2019

I suspect the way it is now gives people just the css necessary for each page which is highest performance. Could we solve the problem updating less? If not, perhaps requiring the css can be the default for optimal performance and disabled with a configuration parameter?

@mrmurphy
Copy link
Author

That's a neat idea! But the require function would have to be moved into a function then, and the user would have call some bootstrapping function before using each component, which feels awkward.

@mrmurphy
Copy link
Author

I'd still vote for letting the user build their own CSS bundle as the simplest and most flexible approach. If the user wants the smallest bundle possible, they can figure out which components are being used, and build their own Antd file just by requiring the components they need.

Not a big deal either way, we've already forked the repo and we're using our fork, so we're not blocked. But I do think the less requirement raises the barrier to entry a bit for other people who want to use this package.

@mrmurphy mrmurphy closed this Sep 1, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants