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

Version1 #3

Merged
merged 16 commits into from
Dec 16, 2022
Merged

Version1 #3

merged 16 commits into from
Dec 16, 2022

Conversation

lgodier
Copy link
Collaborator

@lgodier lgodier commented Dec 9, 2022

No description provided.

@lgodier lgodier requested a review from swouf December 9, 2022 07:50
Copy link
Contributor

@swouf swouf left a comment

Choose a reason for hiding this comment

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

Thank you for this nice first PR!

I asked for a lot of small changes. Please, review as many of them as you can and don't hesitate to ask me questions, either by directly answering my comments or sending me an email. We can also set up a meeting.

The most important comments are the ones on the code that you wrote and that should be taken into account by the end of the project. Try to focus on these then.

And don't be afraid of the number of comments, it is a lot of small things and in general, the code is good.

@@ -1,46 +1,108 @@
# Getting Started with Create React App
# Graasp App Template Typescript
Copy link
Contributor

Choose a reason for hiding this comment

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

For the end of the project, please create a README for your app:

  • Put a description of the app
  • Insert a screenshort
  • Explain a few features that are working and list some features that you proposed to implement (even if you will not do it yourself).

You can make this in another branch. I will valid your PR without it. If you want to do that, please turn this comment into an issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

In the end, I already converted it into an issue. See #4

Comment on lines +3 to +7
"New App Data": "Créer une nouvelle donnée de l'App",
"Update Setting": "Mettre à jour le réglage",
"Local Context": "Contexte local",
"App Context": "Contexte de l'App",
"Members": "Membres"
Copy link
Contributor

Choose a reason for hiding this comment

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

Adapt translations. You can do that later if you want (then turn this comment into an issue).

@swouf swouf added the priority Give priority to this issue label Dec 12, 2022
@swouf swouf added this to the Final presentation of the app milestone Dec 12, 2022
@swouf swouf mentioned this pull request Dec 12, 2022
4 tasks
@lgodier lgodier mentioned this pull request Dec 13, 2022
Copy link
Contributor

@swouf swouf left a comment

Choose a reason for hiding this comment

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

There are missing files.

Copy link
Contributor

@swouf swouf left a comment

Choose a reason for hiding this comment

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

Probably last changes I will request. Good work !

@lgodier
Copy link
Collaborator Author

lgodier commented Dec 14, 2022

done with these new changes

Copy link
Contributor

@swouf swouf left a comment

Choose a reason for hiding this comment

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

You can merge!

@lgodier lgodier merged commit 00a0c15 into main Dec 16, 2022
@spaenleh spaenleh deleted the version1 branch January 5, 2023 17:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority Give priority to this issue v1.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants