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

Adding kotlin-react implementation #1908

Merged
merged 1 commit into from
Jul 13, 2018

Conversation

marcelofrau
Copy link
Contributor

Hello, my team were making some studies with Kotlin for javascript with React and we had found a good way to learn implementing the TodoMVC specification,

Since there is no kotlin alternative in the oficial languages samples, we have found a good idea to propose to you guys our implementation.

We tried to let the code in the most simple way, also we had used the very same source to teach a small group of people (38 people) so the material is in simplified and optimized for learning.

Reason for us to propose the TodoMVC in kotlin-react:

Kotlin is a ascending language, and in the last year it also had being adopted by Google for android development.

Since kotlin is a language that attends the Javascript and frontend enviroment, we think that this version could contribute to TodoMVC , having a kotlin-react alternative for people know that this can be used and it can be very productive to develop.

React is a tendency, and the mix with kotlin-react is growing in the community.

We have not used any that is not officially supported by react or kotlin dependencies. (All dependent library/cli are from jetbrains or react)

We had followed all the documentation provided for TodoMVC official implementation, but if it is not according please let us know if there is something to change to be in conform with TodoMVC specifications.

We hope you guys like our proposal.

Thanks
Best regards,

@rickhallett
Copy link

@marcelofrau Thanks for the submission to TodoMVC. To speed up the evaluation process, are you able to provided a hosted version of the working build?

@marcelofrau
Copy link
Contributor Author

marcelofrau commented Jul 10, 2018 via email

Copy link
Collaborator

@FadySamirSadek FadySamirSadek left a comment

Choose a reason for hiding this comment

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

@marcelofrau Thank you so much for openning this PR. I think this is a very intresting example and a lot of people are going to learn from myself included. specially at the current rate of adoption of Kotlin and as you mentioned Kotlin as a language attends the Javascript and frontend enviroment it makes a lot of sense to be included in the TodoMVC examples. Of course this is not my decision to make. We are currently building a set of guidlines to use to decide on new implementations but I belive yours checks most of the boxes. The only thing I think you should do not is to add your example to the build stages in the travis.yml file to be included in the build also it will make it easier to know if the current implementation pass the Cypress tests.
Thanks and can not wait to land this 🎉

# See https://help.github.com/ignore-files/ for more about ignoring files.

# dependencies
/node_modules
Copy link
Collaborator

Choose a reason for hiding this comment

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

You need to have your node_modules directory so that people can test the examples without having to download a ton of dependencies or even have npm installed. You need to include the basic dependencies that are essential for your app to work and pass the tests.
https://github.com/tastejs/todomvc/blob/master/app-spec.md#dependency-management

Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I forgot that the node_modules was in the .gitignore, will remove and add to the commit.. I also integrated to the cypress tests and are 3 main tests that are still failing, I am fixing these problems and uploading the fixed version as soon as possible.

When the tests pass successfully I'll push the changes and let you know..

Thanks..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hello @FadySamirSadek, I've added the cypress tests integration to pass on kotlin-react and all tests are now passing, also I have commited the node_modules folder, with the necessary dependencies only.

https://travis-ci.org/tastejs/todomvc/jobs/403285541

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please let me know if there is anything more to change.

@marcelofrau marcelofrau force-pushed the kotlin-react branch 2 times, most recently from 846d994 to c5572c1 Compare July 12, 2018 20:22
Copy link
Collaborator

@FadySamirSadek FadySamirSadek left a comment

Choose a reason for hiding this comment

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

Excellent work passing all the tests 👏just a few comments. Can not wait to ship this 🎉.
Thanks

circle.yml Outdated
@@ -34,7 +34,7 @@ defaults: &defaults
path: cypress/screenshots

jobs:
# main JS frameworks
# main JS frameworksl
Copy link
Collaborator

Choose a reason for hiding this comment

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

typo

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oops.. done..

.travis.yml Outdated
@@ -52,6 +52,10 @@ jobs:
# define a separate script for each "examples/*" folder
# this will run it in a separate job on TravisCI
# all these jobs belong to same stage "test" thus will run in parallel
- stage: test
env:
- CYPRESS_framework=kotlin-react
Copy link
Collaborator

Choose a reason for hiding this comment

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

Frameworks are arranged in alphabetical order

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done..

@marcelofrau
Copy link
Contributor Author

@FadySamirSadek All requests are solved..

@FadySamirSadek FadySamirSadek merged commit 7fb200a into tastejs:master Jul 13, 2018
@FadySamirSadek
Copy link
Collaborator

image

@marcelofrau
Copy link
Contributor Author

@FadySamirSadek Do I have to make the change into the index.html file to include the kotlin-react link or you guys do it? If you want I can make another merge request to include it on the list.

@FadySamirSadek
Copy link
Collaborator

@marcelofrau Would appreciate it if you opened another PR to add it, Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants