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: fix the problem that Black background when converting SVG to PNG #41

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

starichat
Copy link

When I parse a style attribute with a global style tag and a stop tag in a Graid component, I will encounter the problem of the above attribute parsing failure,and resulted in the problem that Black background when converting SVG to PNG. I would classify it as the parsing problem of the style attribute, so I submitted this PR to solve the above problem.

@srwiley
Copy link
Owner

srwiley commented Sep 4, 2022

stari, can you please check if pull request #40, Support top level style solves this problem? I just merged it today into the main branch.

@starichat
Copy link
Author

@srwiley , I just merged the above top style code #40 , but I also solved another problem other than GradStop's rgb color not being resolved, you can see the changes I made to StopF function.

@yeldiRium
Copy link
Contributor

@srwiley asked for my input in #40, so I'm giving it:

I don't know a lot about svg gradients, so I can't say for sure that this solution is good. However, the implementation seems plausible to me. Parsing the contents of style tags within a gradient to obtain gradient related information sounds good.
This in addition to the added tests leads me to believe that this PR is good and should be merged.

I am a bit confused about the chinese comments in the tests though. Since the rest of the project is in english, these should probably be translated.
I did translate the comments using google translate and they are things like "read svg" and "define length and width", which are the kinds of comments that can in my opinion be removed entirely.

Apart from that, the PR looks good to me.

@starichat
Copy link
Author

starichat commented Sep 8, 2022

Sorry, I submitted some Chinese comments into the code, accidentally.

I've just removed the comment.

Thanks!

@srwiley
Copy link
Owner

srwiley commented Oct 2, 2022

Sorry for taking so long to get back. I've been busy elsewhere.

I can't accept this pull request as is. The main problem is that in order fix the GradStop bug you have rearranged 8 files. This makes it hard for me to see exactly what you changed because such larger blocks have been moved around. In addition, generated pngs should not be included with source code. Also, I can't see where the fix alters the output from the last merge in the example svgs. You also add the style function to the draw function maps twice, which is unnecessary.

I am sure there is something good here, but please do the minimal alterations to the the existing structure, such as do not split out functions into individual files.

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