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.

Sign up for GitHub

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

Jump to bottom

enhancement: add support for fill in svg #797

Draft
wants to merge 18 commits into
base: main
Choose a base branch
from
Draft

enhancement: add support for fill in svg #797

wants to merge 18 commits into from

Conversation

Aiving
Copy link
Contributor

@Aiving Aiving commented Jul 16, 2024

Closes #796

@Aiving Aiving requested a review from marc2332 as a code owner July 16, 2024 05:31
@marc2332 marc2332 added the enhancement 🔥 New feature or request label Jul 16, 2024
marc2332
marc2332 requested changes Jul 16, 2024
View reviewed changes
Copy link
Owner

@marc2332 marc2332 left a comment

Choose a reason for hiding this comment

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

Why not fill instead of color? The former is used for text

@Aiving
Copy link
Contributor Author

Aiving commented Jul 16, 2024

Why not fill instead of color? The former is used for text

I thought that adding a new property to StyleState, AttributeName/Type etc. would be a bit overkill, but if there is no problem with that I can implement it right now

@Aiving Aiving changed the title enhancement: add support for color in svg enhancement: add support for fill in svg Jul 16, 2024
@Aiving
Copy link
Contributor Author

Aiving commented Jul 16, 2024

@marc2332 done!

@codecov Codecov
Copy link

codecov bot commented Jul 16, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 2.50000% with 39 lines in your changes missing coverage. Please review.

Project coverage is 73.69%. Comparing base (de6d87c) to head (4dbdc29).

Files Patch % Lines
crates/core/src/elements/svg.rs 0.00% 33 Missing ⚠️
crates/state/src/style.rs 0.00% 6 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main     #797      +/-   ##
==========================================
- Coverage   73.81%   73.69%   -0.13%     
==========================================
  Files         201      201              
  Lines       21530    21568      +38     
==========================================
+ Hits        15893    15895       +2     
- Misses       5637     5673      +36

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@marc2332 marc2332 self-requested a review July 19, 2024 17:41
marc2332
marc2332 requested changes Aug 3, 2024
View reviewed changes
Copy link
Owner

@marc2332 marc2332 left a comment

Choose a reason for hiding this comment

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

Can you also add an example?

crates/elements/src/definitions.rs Outdated Show resolved Hide resolved
Aiving and others added 3 commits August 4, 2024 07:14
@Aiving
Merge branch 'main' into feat/support-color-in-svg
b89b6dc
@Aiving
chore(elements/svg): cover all Fill enum patterns
0b8b288 
fix(docs/color-syntax): `rect` -> `red`
feat(docs/attributes/fill): add docs for fill atribute
feat(examples/svg-fill): add example for svg with specified fill color
@Aiving
i forgor
7ff5fb2
@marc2332
Copy link
Owner

marc2332 commented Aug 4, 2024

Does this implementation really make sense?

For example, given this code:

<svg width="525" height="190" viewBox="0 0 525 190" fill="none" xmlns="http://www.w3.org/2000/svg">
    <rect width="250" height="190" fill="#D9D9D9"/>
    <rect x="275.5" y="0.5" width="249" height="189" stroke="black"/>
</svg>

Both rect will have their colors (fill="#D9D9D9" and stroke="black") overrode when using this new fill attribute

Do you think that makes sense?

marc2332
marc2332 requested changes Aug 4, 2024
View reviewed changes
Copy link
Owner

@marc2332 marc2332 left a comment

Choose a reason for hiding this comment

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

btw

crates/elements/src/_docs/attributes/fill.md Outdated Show resolved Hide resolved
@Aiving
Copy link
Contributor Author

Aiving commented Aug 5, 2024

Does this implementation really make sense?

For example, given this code:

<svg width="525" height="190" viewBox="0 0 525 190" fill="none" xmlns="http://www.w3.org/2000/svg">
    <rect width="250" height="190" fill="#D9D9D9"/>
    <rect x="275.5" y="0.5" width="249" height="189" stroke="black"/>
</svg>

Both rect will have their colors (fill="#D9D9D9" and stroke="black") overrode when using this new fill attribute

Do you think that makes sense?

Honestly, I don't think so. Based on the Skia source code, there is a function resolveSvgColor where some fPresentationContext is used, where you can set the color used for currentColor. However, it is in SkSVGRenderContext which I can't find in skia-safe. In case I do, I will try to make a PR in skia-safe that adds the SkSVGRenderContext.

@marc2332 marc2332 marked this pull request as draft August 18, 2024 10:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@marc2332 marc2332 marc2332 requested changes

Assignees
No one assigned
Labels
enhancement 🔥 New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

enhancement: Support for fill attribute in svg
2 participants
@Aiving @marc2332