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

C# Transpiler, API, and Tests #129

Merged
merged 1 commit into from
Sep 23, 2015
Merged

Conversation

pragmatrix
Copy link
Contributor

This pull request adds a C#/.NET transpiler, API, and tests to the css-layout project. The transpiler is integrated by adding a few lines to transpile.js and the API is very similar to the Java version.

Details

Formatting

Even though the contribution guide states otherwise, the C# code is indented by 4 spaces instead of 2, and { braces are usually placed on the next line. I think that keeping the default Visual Studio settings for C# code will make the code easier to maintain, also I've added an .editorconfig file so that other editors can pick up the correct format.

Idiomatic CSharp

All public names of classes, methods, and members are in PascalCase. Properties were used when they seemed appropriate.

Enum members defined by CSSAlign, CSSConstants, CSSDirection, CSSFlexDirection, CSSJustify, CSSPositionType, and CSSWrap have been converted to PascalCase. CSharpTranspiler.js will take care of the case conversion for the generated code.

Method forwarders / duplication

To keep the differences between the Java and C# transpiler minimal, I decided to leave some internal methods in camlCase. When appropriate, these methods were implemented as extension methods.

Visual Studio Solution

The solution's CSSLayout project is located in src/csharp and produces a portable class library Profile 259, which supports the .NET Framework 4.5, Windows 8+, Windows Phone 8.1, Windows Phone Silverlight 8, and Xamarin.Android, Xamarin.iOS, and Xamarin.iOS (Classic API).

The solution also includes the test project, which uses NUnit.

The code uses C# 6 syntax.

File Format

All *.cs files are encoded in UTF-8, LF line endings, no BOMs. *.sln, *.csproj, and *.config files are in UTF-8 with BOMs and CRLF line endings.

File Notes

CSSNode.cs

  • All methods are non-virtual so far. This is because I don't know which methods are intended to be virtual and which are not, so feedback is very appreciated here. For the projects I've done with the C# version, inheriting from CSSNode was not required.
  • The names of the properties for accessing the style width and height are named just Width and Height instead of StyleWidth and StyleHeight. All properties except the ones beginning with Layout are meant to be style related.
  • I've added Min/Max-Height/Width properties to CSSNode for reading and modifying CSSStyle fields because the CSSNode.style member is not accessible through CSSNode yet.
  • The measure function is defined as a .NET delegate.

Spacing.cs

This class seems not to be indented to be public, but gets exposed in CSSNode in the Java API through a field named padding. I decided to remove CSSNode.padding for now, because padding can already be modified with CSSNode's methods.

CSSSpacingType.cs

I did not liked the idea of using integers for the spacing type, so I've introduced an enum named CSSSpacingType, which is used in CSSNode for specifying the various types of spacing for paddings, margins, and borders.

Build Script

Only the transpiler is run from the gruntfile. However, there is a Makefile in the src/csharp directory that is used to build & test the solution and pack & upload the NuGet package.

NuGet Package

This pull request is based on an unofficial adoption, which I am maintaining for some time. Also there is a NuGet package available that is named Facebook.CSSLayout. I am happy to transfer ownership, if this pull request is accepted.

I've decided to properly clean up the API before creating this pull request, thereby introducing (hopefully very minor) incompatibilities with code written against the NuGet packages.

And sorry, no history, I've never rebased the csharp version, but will from now on.

vjeux added a commit that referenced this pull request Sep 23, 2015
@vjeux vjeux merged commit 4ca2ea3 into facebook:master Sep 23, 2015
@vjeux
Copy link
Contributor

vjeux commented Sep 23, 2015

That's awesome, thanks for doing that work :)

@ColinEberhardt
Copy link
Contributor

Very awesome @pragmatrix :-)

A few things that I am wondering about ..

  1. This should be added to the build - currently the grunt-based build lints, transpiles, runs the tests (in all languages), then packages the code for distribution. I'd love to see the C# code tested here also. This build is also part of the Travis-CI, which ensures Pull Requests do not break the code.
  2. It would also be great to have this added to the release process - my aim has been that tagged builds form a semantically versioning release of all the different transpiled versions of the code.

Of the above (1) should be quite easy. And for (2) I guess it would just require documentation. Can NuGet packages be released by a command line tool similar to the way npm works?

@pragmatrix
Copy link
Contributor Author

Yes, nuget.exe push releases a nuget package given an api key was set up on the local machine with nuget.exe setapikey. Here is the line in the Makefile that releases the package.

@woehrl01
Copy link
Contributor

thanks @pragmatrix !

do you mind changing the syntax to c# 5? Not everyone can use the latest VS.
E.g. I'm using this library (manual java port) in a .NET 4.0 VS2012 project so I have to compile the stuff by my self anyway. But changing the syntax everytime is a little bothering :)

I think this makes the library a little more re-usable.

@pragmatrix
Copy link
Contributor Author

@woehrl01 No I don't mind, Resharper got me there :) Can you do the pull request? And would a .NET 4.0 compatible NuGet package fit into your project?

@pragmatrix
Copy link
Contributor Author

@woehrl01 I had some time to see how hard it would be to get rid of using static, but since only a few constants were referenced from CSSLayout, the changes were minimal. I've prepared a branch here: https://github.com/pragmatrix/css-layout/tree/csharp5 . Please take a look if this compiles properly with VS2012.

@woehrl01
Copy link
Contributor

@pragmatrix Awesome! Compiles perfectly, here. I just had to reference System to make this line work.

@pragmatrix pragmatrix deleted the csharp-fb-pr branch October 5, 2015 05:36
@browniefed browniefed mentioned this pull request Sep 16, 2016
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