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

Add Windows support #1632

Merged
merged 83 commits into from
Jul 15, 2022
Merged

Conversation

marlenecota
Copy link
Contributor

Summary

Explain the motivation for making this change: here are some points to help you:

Test Plan

Demonstrate the code is solid. Example: The exact commands you ran and their output, screenshots / videos if the pull request changes UI.

SvgDemo_Part1.mp4
SvgDemo_Part2.mp4

Compatibility

OS Implemented
iOS
Android
Windows

Checklist

  • I have tested this on a device and a simulator
  • I added documentation in README.md
  • I updated the typed files (typescript)
  • I added a test for the API in the __tests__ folder

@guillemonzo
Copy link

Any updates on this?

@jdk
Copy link

jdk commented Feb 22, 2022

@PPatBoyd @marlenecota can this be merged?

@marlenecota you mentioned a parity document, was wondering if I missed this?

@markokhman
Copy link

Did anybody try to merge this?

@markokhman
Copy link

Guys, please let me know what is this issue up to? I can contribute by testing and debugging.
Seems like this is about to be ready for merging.

@jdk
Copy link

jdk commented Apr 3, 2022

@msand We've tested this too. Is there anything we can further do to help expedite a merge?

Add windows to "files" and "keywords"
@shanelile
Copy link

I'd also like to know what the status here is - is there anything folks can do to help out with this?

@marlenecota
Copy link
Contributor Author

marlenecota commented May 24, 2022

I'd also like to know what the status here is - is there anything folks can do to help out with this?

Text, TSpan, TextPath, ClipPath, Mask, Marker, and ForeignObject have been put on the back burner, but this could be merged as is for a partial implementation. Need someone with write access to be ok with that and approve it.

@ryanlntn
Copy link

@marlenecota I'm seeing this when trying to build for ARM64

Screen Shot 2022-06-15 at 8 55 38 AM

I tried running NuGet package restore but still get the same error. Any idea what I might be missing?

@alexzobi
Copy link

@Saadnajmi @asklar @PPatBoyd sorry to be annoying, but would it be possible to take a peak at this? Even this partial implementation would be a huge boon for me and it looks like many others. @marlenecota thanks for all of the hard work!

@Saadnajmi
Copy link
Contributor

@Saadnajmi @asklar @PPatBoyd sorry to be annoying, but would it be possible to take a peak at this? Even this partial implementation would be a huge boon for me and it looks like many others. @marlenecota thanks for all of the hard work!

Software Mansion maintains React Native SVG now, so I think the person who could comment on whether this pointed up with their future plans for the repo is @WoLewicki . I know they were working on bringing Fabric support.

@WoLewicki
Copy link
Member

I have almost no knowledge of Windows development so if you feel like it is ready to be merged, I am OK with that and we can do it for sure. We are working on bringing Fabric to the repo, but I am also maintainer of RNScreens and there are still some issues with Fabric there so unfortunately I didn't spend much time on RNSVG lately.

@WoLewicki WoLewicki mentioned this pull request Jun 20, 2022
@gillpeacegood
Copy link

@marlenecota it seems it could be merged back?

@jdk
Copy link

jdk commented Jun 30, 2022

@marlenecota 👍

Tested here too. Please. It’s ready.

@taduyde
Copy link

taduyde commented Jul 3, 2022

ready to merge?

@marlenecota
Copy link
Contributor Author

@jdk - which version of RN/RNW did you test?

This was created and tested in 0.64. I am currently spread too thin with other work to validate newer versions. If anyone has been able to validate this change against newer versions or can volunteer time to do so, please report results back on this thread.

If we want to go ahead with the merge, any upgrading issues could be addressed in future PRs.

@jdk
Copy link

jdk commented Jul 7, 2022

@jdk - which version of RN/RNW did you test?

This was created and tested in 0.64. I am currently spread too thin with other work to validate newer versions. If anyone has been able to validate this change against newer versions or can volunteer time to do so, please report results back on this thread.

If we want to go ahead with the merge, any upgrading issues could be addressed in future PRs.

"react-native": "0.67.4",
"react-native-windows": "0.67.7"

@jeanmaried
Copy link

jeanmaried commented Jul 11, 2022

Working on:
"react-native": "0.67.2",
"react-native-windows": "0.67.7"

If we could please get this merged 🙏🏻

@WoLewicki
Copy link
Member

@marlenecota if you are OK with merging it, we can do it and then release a new version of library so people can test it thoroughly.

@lcherukuri
Copy link

When can we expect this to merge and release?

@marlenecota
Copy link
Contributor Author

marlenecota commented Jul 14, 2022

@marlenecota if you are OK with merging it, we can do it and then release a new version of library so people can test it thoroughly.

@WoLewicki sounds good!

Copy link
Member

@WoLewicki WoLewicki left a comment

Choose a reason for hiding this comment

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

Great job! Let's merge it 🚀

for (size_t i = 0; i < stops.size(); ++i) {
Brushes::CanvasGradientStop stop{};
stop.Position = Utils::JSValueAsFloat(stops.at(i));
stop.Color = Utils::JSValueAsColor(stops.at(++i));
Copy link
Member

Choose a reason for hiding this comment

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

So since #1726 was merged, it should work correctly then @marlenecota ?

@WoLewicki WoLewicki merged commit 523568c into software-mansion:develop Jul 15, 2022
WoLewicki pushed a commit that referenced this pull request Jul 15, 2022
Adds Windows support.

Co-authored-by: Adam Gleitman <[email protected]>
Co-authored-by: REDMOND\agnel <[email protected]>
@Saadnajmi
Copy link
Contributor

Thank you @WoLewicki ! I'm sure this will make a lot of people happy and their lives easier working across platforms=)

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.