Skip to content

Fit container - #19

Open
bung87 wants to merge 5 commits into
albinotonnina:masterfrom
bung87:fit-container
Open

Fit container#19
bung87 wants to merge 5 commits into
albinotonnina:masterfrom
bung87:fit-container

Conversation

@bung87

@bung87 bung87 commented Nov 26, 2019

Copy link
Copy Markdown

No description provided.

Comment thread lib/__tests__/asset.js
it("should ouput the optimized svg code...", async () => {
// Init bundler
const bundler = new Bundler(path.join(__dirname, './llama.svg'), {
const bundler = new Bundler(path.join(__dirname, './llama.isvg'), {

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

why isvg?

Comment thread lib/index.js
@@ -1,3 +1,3 @@
module.exports = function (bundler) {
bundler.addAssetType('svg', require.resolve('./asset.js'));
bundler.addAssetType('isvg', require.resolve('./asset.js'));

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

are you proposing to replace svg with isvg?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

yes, using svg chould conflict with other technic, when developer write svg path in scss , vue or something, this project has big difference between file loader , it does not produce a new file, so make it has a unique extension avoiding conflict with other file loader, also sound reasonable.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

unless the asset extension hook can have known the svg module import from which kind of file or in which way, this is the simple solution.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Yeah the isvg thing seems odd to me, but it is pretty intuitive, as long as it's documented properly... My only issue is that I would be pulling .svg files from another package, and I would not be able to change the file extension within that package without copying the SVGs out...

@bung87, can you give a code example that shows where this plugin would mess up usage of .svg in SCSS or Vue? I'm not sure I understand, and it might be something worth elaborating on in the documenting for the .isvg extension.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

@lazd it's described in #13
as it's a asset any other loader would transpile the svg module to a file path when you import it, you can simply create a project that contains scss , set a background to a svg file,then in js file import it, or in vue SCF write style refers to it , as the developer intend to refers a file, but this plugin is about generate inline code , that's not the right use case, also other plugin process would give a path as result.

I understand your use case , it make sense.
if remains the svg extension, this plugin should detect user import it in which file, it makes more complicated, since other process would interupt, or like webpack add some command in file path when user import it xxx.svg!inline or something.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

about your use case ,maybe can use .flowconfig module.name_mapper https://parceljs.org/module_resolution.html#flow-with-absolute-or-tilde-resolution

@lazd

lazd commented Dec 5, 2019

Copy link
Copy Markdown

@albinotonnina what are we thinking on this one? It looks like it might make this plugin usable again...

@albinotonnina

Copy link
Copy Markdown
Owner

@lazd, @bung87 thanks for the work you've doing on this.
I am not actively following the development of Parcel atm so I'm a bit rusty here.
I am aware that this plugin is not working very well with the latest versions and I do remember ending up with a conclusion similar to @bung87 proposal, for reasons I cannot even recall.

@lazd so you say that this PR with the new constrain of a custom extension for svgs would make the plugin usable again? Then I'm in.

But I'm a bit confused now with the 2 PRs @bung87 opened.
Would you be able to consolidate everything in one PR (unless needed otherwise) and:

  • have tests pass, add more if you feel like it's needed.
  • update the documentation with the new requirement?

Thank you

@bung87

bung87 commented Dec 7, 2019

Copy link
Copy Markdown
Author

2336558 make svg scale to container, and update to latest svgo as dependency, I'm not sure make it default is right or may have better way to archive this. and it fix the tests.

pripor commit just fix #9 #13.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants