-
Notifications
You must be signed in to change notification settings - Fork 375
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
feat(examples): add todolist package & realm #1811
Conversation
cc @gnolang/devrels |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1811 +/- ##
==========================================
- Coverage 47.79% 45.08% -2.71%
==========================================
Files 393 464 +71
Lines 61654 68008 +6354
==========================================
+ Hits 29465 30662 +1197
- Misses 29717 34772 +5055
- Partials 2472 2574 +102 ☔ View full report in Codecov by Sentry. |
To devteam: it's a little bit difficult to understand all the tests, and to read the results. Some errors were repeated 2 times (2 tests gave the same errors). |
added a fix to ufmt so that the unicode emojis can be displayed correctly, pushed to my fork. I don't know if the merge will add that modification too |
Hey @MalekLahbib, thanks for your work. I see that this PR also includes both the todo-list app, as well as the ufmt multi-byte fix. Can you please separate this into two PRs, so that the PRs are scoped correctly? I suggest you checkout a new branch from master and implement the ufmt change there, while deleting the change here and only working on the todo-list app. |
Hey @leohhhn, thanks for your message, a lot of github things are new to me, so I'm still learning. I noticed when I pushed the ufmt solution that it was on the same PR and didn't know what to do. I 'm gonna try to figure out what you told me to do by myself. If I have difficulties I'll ask for help. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for providing this example! Please resolve the comments before we can merge :)
Also, you can consider adding tests for the realm itself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the contribution 🙏
@MalekLahbib Please check why the cc @leohhhn |
I sent on signal about this issue, @leohhhn asked me to commit so he can have my code. Here's a screenshot of what I have as an error msg: |
@leohhhn @zivkovicmilos all tests passed this time, can you check again and tell me if there's other things to modify? the realm test file worked locally for me. Thanks |
Contributors' checklist...
BREAKING CHANGE: xxx
message was included in the description