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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix basetag bug #664

Closed
wants to merge 4 commits into from
Closed

Fix basetag bug #664

wants to merge 4 commits into from

Conversation

Fuzzyma
Copy link
Member

@Fuzzyma Fuzzyma commented Apr 23, 2017

prefix all url() references with current location (#568)

the prefix is removed on export and added on import.

TODO:

  • add tests when exporting svg which alrady has a location in fill set e.g. fill="url(google.de/some.svg# someId)" and the same when importing
  • compare performance (which is most likely reduced when importing/exporting svgs)

the prefix is removed on export and added on import
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.1%) to 98.829% when pulling 86f2910 on fix-basetag-bug into 54362a8 on 3.0.0.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.1%) to 98.829% when pulling 1d532f2 on fix-basetag-bug into 54362a8 on 3.0.0.

@Fuzzyma
Copy link
Member Author

Fuzzyma commented Apr 23, 2017

I thought about that, too. But when some genius people start creating the basetag with javascript we are lost again. But one might argue that this is the users fault then because he should know better.

@wout
Copy link
Member

wout commented Apr 23, 2017

True. This solution covers all use cases. I think it's good to go then. We might be missing some attributes using an URL, but that's something for later.

@Fuzzyma
Copy link
Member Author

Fuzzyma commented Apr 23, 2017

Atm every attribute which matches url(...) is corrected

@Fuzzyma
Copy link
Member Author

Fuzzyma commented Apr 23, 2017

But I guess thats not enough. use uses href which doesnt containa any url(). Its just the plain address. I need to check that, too.

src/regex.js Outdated
@@ -39,6 +39,9 @@ SVG.regex = {
// Test for image url
, isImage: /\.(jpg|jpeg|png|gif|svg)(\?[^=]+.*)?/i

// Test for url reference
, isUrl: /url\(([-\w:/]+(?:\.\w+)?)?#([-\w]+)\)/
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think there might be some cases that this regexp is not matching, like url(https://example.org/absolute/URI/with/absolute/path/to/resource.txt#theid). Also, I was wondering if we could use a simpler regexp instead, something like this: /url\((.*)#([-\w]+)\)/. We keep it simple and just check if there is something in front of the id. If there is nothing, then we had the complete url. I don't know if it is worth it to try to detect if the path before the id is absolute or relative.

Copy link
Member Author

Choose a reason for hiding this comment

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

I like to be loose as possible by avoing the .* operator. I will go for /(url\()?([-\w:/.]+)?#([-\w]+)/ which should match everything valid.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.1%) to 98.832% when pulling 9660844 on fix-basetag-bug into 54362a8 on 3.0.0.

@Fuzzyma
Copy link
Member Author

Fuzzyma commented Apr 25, 2017

Seriously I dont like this...
I added like 150 lines of code and 5 helper functions only to achieve the proper workaround for this few poeple who uses a basetag. I would most likely prefer stating in the docs that people should avoid the basetag and if they have to use it, install a plugin instead.
This is so much overhead for literally nothing...

@wout
Copy link
Member

wout commented Apr 25, 2017

Absolutely agree. Let's poor it into a plugin.

@Fuzzyma Fuzzyma closed this Apr 25, 2017
@RmiTtro
Copy link
Collaborator

RmiTtro commented Apr 26, 2017

I 100% agree!

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.

4 participants